Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Condition #184

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Improve Condition #184

wants to merge 2 commits into from

Conversation

Atry
Copy link
Contributor

@Atry Atry commented Mar 14, 2022

  1. Introduced the ADT type ConditionState to replace ?Awaitable<T> in the original implementation, removing HH_FIXME[4110]
  2. Added a Finish state to remove the reference from the child awaitable to the Condition, in case of memory leak
  3. Added wait_for_notification_async and ConditionNotifyee to hide the setState function

Test Plan:

./minitest.sh tests/async/ConditionTest.php
Starting ConditionTest...
--- ConditionTest::runAsync()
-----   ConditionTest::testSucceedFirst()
-----   ConditionTest::testFailFirst()
-----   ConditionTest::testSucceedLater()
-----   ConditionTest::testFailLater()
OK: ConditionTest

@facebook-github-bot
Copy link
Contributor

@Atry has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@Atry
Copy link
Contributor Author

Atry commented Mar 14, 2022

I am not sure about the memory leak part.

Given the following code

async function outer(): Awaitable<void> {
  await wait_for_notification_async(
    async $notifyee ==> {
      concurrent {
        await async {
          await gen_usleep(100);
          $notifyee->trySucceed("fast condition");
        }
        await async {
          await gen_usleep(10000000000);
          $notifyee->trySucceed("slow condition");
        }
      }
    }
  );
}

I want the unfinished "slow condition" not to prevent the outer awaitable from being garbage-collected. Not sure if the current implementation would.

@facebook-github-bot
Copy link
Contributor

@Atry has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@Atry has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@Atry Atry requested a review from jano March 14, 2022 18:22
Copy link
Contributor

@fredemmott fredemmott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for small nits and potential for enum classes. Naming TBC.

Will need www performance checks (will comment on internal diffs)

Overall like the removal of the fixme and more explicit states.


namespace HH\Lib\_Private;

use type HH\Lib\Async\Condition;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't use use type or use function in the HSL outside of tests; we want use namespace HH\Lib\Async;

AsyncResult::class,
Finished::class,
)>>
interface ConditionState<T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • this API feels a lot like a Promise; would that be a better naming?
  • this + the interfaces potentially feels like a good use case for enum class - would that make sensehere?

Copy link
Contributor Author

@Atry Atry Mar 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AsyncResult and SyncResult cannot be enum classes because they contain properties. Finished and NotStarted can be turned into enum classes.

/* HH_FIXME[4110]: Type error revealed by type-safe instanceof feature. See https://fburl.com/instanceof */
$this->condition->succeed($result);
if (!$this->state->trySucceed($this, $result)) {
invariant_violation('Unable to notify Condition twice');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use invariant($condition, $message) instead

);
$this->condition->fail($exception);
if (!$this->state->tryFail($this, $exception)) {
invariant_violation('Unable to notify Condition twice');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

return $condition->waitForNotificationAsync($notifiers($condition));
}

interface ConditionNotifyee<-T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly feels like a Promise API

Copy link
Contributor Author

@Atry Atry Mar 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used to name it Promise but the API is different from Promise in either Scala or JavaScript. The current version named it ConditionNotifyee because it is used with the wait_for_notification_async API.

The primary purpose of this interface is to hide the setState function from public API.

): Awaitable<T> {
$condition = new Condition();
return $condition->waitForNotificationAsync($notifiers($condition));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • at a minimum, this needs a similar 'do not use' comment to Async\Poll
  • AIUI this is a control flow that we specificially do not want to make easier/more straightforward, due to ease of misuse (e.g. it can make it easy to make your request 10% faster by reducing server concurrent requests by 20%, in a way that doesn't show up in request-isolated benchmarking)

@fredemmott
Copy link
Contributor

I am not sure about the memory leak part.

Given the following code

async function outer(): Awaitable<void> {
  await wait_for_notification_async(
    async $notifyee ==> {
      concurrent {
        await async {
          await gen_usleep(100);
          $notifyee->trySucceed("fast condition");
        }
        await async {
          await gen_usleep(10000000000);
          $notifyee->trySucceed("slow condition");
        }
      }
    }
  );
}

I want the unfinished "slow condition" not to prevent the outer awaitable from being garbage-collected. Not sure if the current implementation would.

It will not block outer being garbage collected, but the async and concurrent blocks can not complete until the slow condition completes, so they - and anything they depend on - will not complete.

Also an invariant violation will always trigger in this example.

The 'good' uses of awaiting in a loop, and async generators are when everything will eventually be processed, but need to be processed in order. Most uses are inappropriate and should be Vec\map_async or similar.

The 'good' uses of AsyncPoll are when everything passed in will eventually be awaited, but execution is independent - similar to a scheduler without cancellation. In general, 'start but do not finish' or 'ignore everything except the first result' is bad for site health, and not something we want to support in hack - but unfortunately the primitives we need to support the 'good' cases also enable the 'bad' cases.

@Atry
Copy link
Contributor Author

Atry commented Mar 15, 2022

Also an invariant violation will always trigger in this example.

I think it would not trigger an invariant violation because it uses trySucceed instead of succeed.

@fredemmott
Copy link
Contributor

Still not sure it's desirable, but if a higher level function is needed, you don't need the notifier to be part of the public API: function can either return or throw, either will be considered resolving the awaitable.

There isn't a need for explicit succeed/fail calls, and probably isn't even a need for explicit catching - the exception will be stored in the awaitable by default.

This would also remove the possibility of weirdness if e.g. people don't return immediately after indicating success or failure

1. Introduce the ADT type ConditionState to replace ?Awaitable<T>, suppressing HH_FIXME[4110]
2. Added a Finish state to remove the reference from the child awaitable to the Condition, in case of memory leak
3. Added wait_for_notification_async and ConditionNotifyee to hide the setState function
@Atry
Copy link
Contributor Author

Atry commented Mar 15, 2022

This PR had multiple purposes. I just split the trySucceed/tryFail changes into #185.

@Atry Atry marked this pull request as draft March 15, 2022 20:05
@facebook-github-bot
Copy link
Contributor

@Atry has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

Hi @Atry!

Thank you for your pull request.

We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants