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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 139 additions & 0 deletions src/_Private/ConditionState.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
<?hh
/*
* Copyright (c) 2004-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the hphp/hsl/ subdirectory of this source tree.
*
*/

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;


<<__Sealed(
NotStarted::class,
SyncResult::class,
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.

public function waitForNotificationAsync(
Condition<T> $condition,
Awaitable<void> $notifiers,
): Awaitable<T>;
public function trySucceed(Condition<T> $condition, T $result): bool;
public function tryFail(Condition<T> $condition, \Exception $exception): bool;
}

final class NotStarted<T> implements ConditionState<T> {
private function __construct() {}
<<__Memoize>>
public static function getInstance(): this {
return new self();
}

public async function waitForNotificationAsync(
Condition<T> $condition,
Awaitable<void> $notifiers,
): Awaitable<T> {
$handle = ConditionWaitHandle::create($notifiers);
$condition->setState(new AsyncResult($handle));
try {
return await $handle;
} finally {
$condition->setState(Finished::getInstance());
}
}

public function trySucceed(Condition<T> $condition, T $result): bool {
$condition->setState(
new SyncResult(
async {
return $result;
},
),
);
return true;
}
public function tryFail(
Condition<T> $condition,
\Exception $exception,
): bool {
$condition->setState(
new SyncResult(
async {
throw $exception;
},
),
);
return true;
}
}

final class AsyncResult<T> implements ConditionState<T> {
public function __construct(private ConditionWaitHandle<T> $resultHandle) {}
public function waitForNotificationAsync(
Condition<T> $_state_ref,
Awaitable<void> $_notifiers,
): Awaitable<T> {
invariant_violation('Unable to wait for notification twice');
}
public function trySucceed(Condition<T> $condition, T $result): bool {
$this->resultHandle->succeed($result);
return true;
}
public function tryFail(
Condition<T> $condition,
\Exception $exception,
): bool {
$this->resultHandle->fail($exception);
return true;
}

}

final class SyncResult<T> implements ConditionState<T> {
public function __construct(private Awaitable<T> $resultAwaitable) {}
public function waitForNotificationAsync(
Condition<T> $condition,
Awaitable<void> $_notifiers,
): Awaitable<T> {
$condition->setState(Finished::getInstance());
return $this->resultAwaitable;
}

public function trySucceed(Condition<T> $condition, T $result): bool {
return false;
}
public function tryFail(
Condition<T> $condition,
\Exception $exception,
): bool {
return false;
}
}

final class Finished<T> implements ConditionState<T> {
private function __construct() {}
<<__Memoize>>
public static function getInstance(): this {
return new self();
}
public function waitForNotificationAsync(
Condition<T> $_state_ref,
Awaitable<void> $_notifiers,
): Awaitable<T> {
invariant_violation('Unable to wait for notification twice');
}
public function trySucceed(Condition<T> $condition, T $result): bool {
return false;
}
public function tryFail(
Condition<T> $condition,
\Exception $exception,
): bool {
return false;
}
}
111 changes: 76 additions & 35 deletions src/async/Condition.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,46 +10,42 @@

namespace HH\Lib\Async;

use namespace HH\Lib\_Private;

/**
* A wrapper around ConditionWaitHandle that allows notification events
* to occur before the condition is awaited.
*/
class Condition<T> {
private ?Awaitable<T> $condition = null;
class Condition<T> implements ConditionNotifyee<T> {
private _Private\ConditionState<T> $state;
public function __construct() {
$this->state = _Private\NotStarted::getInstance();
}

public function setState(_Private\ConditionState<T> $state): void {
$this->state = $state;
}

/**
* Notify the condition variable of success and set the result.
*/
final public function succeed(T $result): void {
if ($this->condition === null) {
$this->condition = async {
return $result;
};
} else {
invariant(
$this->condition is ConditionWaitHandle<_>,
'Unable to notify AsyncCondition twice',
);
/* HH_FIXME[4110]: Type error revealed by type-safe instanceof feature. See https://fburl.com/instanceof */
$this->condition->succeed($result);
}
invariant(
$this->state->trySucceed($this, $result),
'Unable to notify Condition twice',
);
}

/**
* Notify the condition variable of failure and set the exception.
*/
final public function fail(\Exception $exception): void {
if ($this->condition === null) {
$this->condition = async {
throw $exception;
};
} else {
invariant(
$this->condition is ConditionWaitHandle<_>,
'Unable to notify AsyncCondition twice',
);
$this->condition->fail($exception);
}
invariant(
$this->state->tryFail($this, $exception),
'Unable to notify Condition twice',
);
}

final public function trySucceed(T $result): bool {
return $this->state->trySucceed($this, $result);
}

final public function tryFail(\Exception $exception): bool {
return $this->state->tryFail($this, $exception);
}

/**
Expand All @@ -66,9 +62,54 @@ final public function fail(\Exception $exception): void {
final public async function waitForNotificationAsync(
Awaitable<void> $notifiers,
): Awaitable<T> {
if ($this->condition === null) {
$this->condition = ConditionWaitHandle::create($notifiers);
}
return await $this->condition;
return await $this->state->waitForNotificationAsync($this, $notifiers);
}
}


/**
* Asynchronously wait for the condition variable to be notified and
* return the result or throw the exception received via notification.
*
* The caller must provide an Awaitable $notifiers (which must be a
* WaitHandle) that must not finish before the notification is received.
* This means $notifiers must represent work that is guaranteed to
* eventually trigger the notification. As long as the notification is
* issued only once, asynchronous execution unrelated to $notifiers is
* allowed to trigger the notification.
*/
function wait_for_notification_async<T>(
(function(ConditionNotifyee<T>): Awaitable<void>) $notifiers,
): 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)


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.


/**
* Notify the condition variable of success and set the $result.
*/
public function succeed(T $result): void;
/**
* Notify the condition variable of success and set the $result.
*
* @return
* true if the condition is set to $result successfully, false if the
* condition was previously set to another result or exception.
*/
public function trySucceed(T $result): bool;

/**
* Notify the condition variable of failure and set the exception.
*/
public function fail(\Exception $exception): void;
/**
* Notify the condition variable of failure and set the $exception.
*
* @return
* true if the condition is set to $exception successfully, false if the
* condition was previously set to another result or exception.
*/
public function tryFail(\Exception $exception): bool;
}