Feature/add promisesraces cancel mechanism #39

kronostof · 2019-12-23T15:51:35Z

Give possibility to cancel a promise, to stop the job necessary to resolve this promise.

Related to this issues

…definition

src/Adapter/Amp/EventLoop.php

b-viguier

Looks… promising!! (Joke inside 😉 )
This is a partial review, I still have to read internal implementations, and check "complex" tests.
Good job, keep going :)

b-viguier · 2019-12-24T12:23:24Z

examples/05-cancellable-promise.php

+    $result = 'cancelled promise';
+} catch (\Exception $e) {
+    $result = 'other exception';
+}


Do we agree that this example should not throw any exception? $result should be equal to ["not resolved", "B"] right?
If so, can you remove the last try/catch block around the wait? It's sound confusing to me

examples/06-advanced-cancellable-promise.php

b-viguier · 2019-12-24T12:25:17Z

examples/06-advanced-cancellable-promise.php

+        echo "[$id] Working after cancellation !\n";
+        yield $eventLoop->delay($time);
+        echo "[$id] Working after cancellation !\n";
+        yield $eventLoop->delay($time);


What about a loop?

b-viguier · 2019-12-24T12:26:04Z

examples/06-advanced-cancellable-promise.php

+$eventLoop = new Adapter\Amp\EventLoop();
+//$eventLoop = new Adapter\ReactPhp\EventLoop(new React\EventLoop\StreamSelectLoop());
+
+function timer(EventLoop $eventLoop, string $id, int $time, \M6Web\Tornado\Promise &$promise = null)


Why a reference to an object? Object are already passed by reference in Php 🤔

b-viguier · 2019-12-24T12:26:21Z

examples/05-cancellable-promise.php

+//$eventLoop = new Adapter\Amp\EventLoop();
+//$eventLoop = new Adapter\ReactPhp\EventLoop(new React\EventLoop\StreamSelectLoop());
+
+function timer(EventLoop $eventLoop, string $id, int $time, \M6Web\Tornado\Promise &$promise = null)


Why a reference to an object? Object are already passed by reference in Php 🤔

b-viguier · 2019-12-24T12:34:30Z

src/Promise.php

@@ -7,4 +7,5 @@
 */
 interface Promise
 {
+    public function cancel();


Suggested change

public function cancel();

/**

* Requests the cancellation of the promise.

* Functions still yielding for this promise will receive a `CancelledException`.

*/

public function cancel(string $reason): void;

Since php 7.1, use void when nothing is returned.

What about the $reason parameter? To be forwarded in the exception (and documented)

b-viguier · 2019-12-24T12:36:13Z

tests/CancellableEventLoopTest.php

+
+namespace M6WebTest\Tornado;
+
+abstract class CancellableEventLoopTest extends EventLoopTest


What is the purpose of this new class? It doesn't define any abstract function… so why not include the trait and the constant in the base EventLoopTest? 🤔

tests/EventLoopTest/CancellationTest.php

src/Adapter/Amp/EventLoop.php

b-viguier · 2019-12-24T12:45:41Z

src/Adapter/Amp/EventLoop.php

+            foreach ($promises as $promise) {
+                $promise->cancel();
+            }
+        };


This behavior should be clearly documented in function DocBlock: If you cancel the returned promise, every sub-promise will be cancelled too
Same for every function

… and must be tested 😅

b-viguier

AMP / Guzzle / ReactPhp adapters review

b-viguier · 2020-01-02T08:57:28Z

src/Adapter/Amp/EventLoop.php

+            foreach ($promises as $promise) {
+                $promise->cancel();
+            }
+        };


… and must be tested 😅

b-viguier · 2020-01-02T09:45:50Z

src/Adapter/Amp/EventLoop.php

            },
            $promises
        );

        foreach ($promises as $index => $promise) {
            \Amp\Promise\rethrow(new \Amp\Coroutine($wrapPromise($promise)));
        }
+        $promisesCancellation = function () use (&$toWrapPromises) {
+            foreach ($toWrapPromises as $index => $promise) {


Suggested change

foreach ($toWrapPromises as $index => $promise) {

foreach ($toWrapPromises as $promise) {

b-viguier · 2020-01-02T09:48:57Z

src/Adapter/Amp/EventLoop.php

@@ -121,6 +137,8 @@ public function promiseForeach($traversable, callable $function): Promise
     */
    public function promiseRace(Promise ...$promises): Promise
    {
+        $toWrapPromises = [];
+        $promisesCancellation = null;


Why pre declaring this variable? Looks confusing…

b-viguier · 2020-01-02T09:51:32Z

src/Adapter/Amp/EventLoop.php

@@ -121,6 +137,8 @@ public function promiseForeach($traversable, callable $function): Promise
     */
    public function promiseRace(Promise ...$promises): Promise
    {
+        $toWrapPromises = [];


This is confusing… what about using original $promises array and use a new name like ampPromises for array_map result?

b-viguier · 2020-01-02T09:58:54Z

src/Adapter/Amp/EventLoop.php

+        };
+
+        $cancellation = function () use (&$deferred, &$promisesCancellation) {
+            $deferred->fail(new CancelledException('promise race cancellation'));


We should use the cancellation reason provided when cancelling the promise

b-viguier · 2020-01-02T15:21:57Z

src/Adapter/Amp/EventLoop.php

@@ -232,7 +281,9 @@ function ($watcherId, $stream) use ($deferred) {
            }
        );

-        return Internal\PromiseWrapper::createUnhandled($deferred->promise(), $this->unhandledFailingPromises);
+        $cancellation = function () {};


onReadable returns a watcher ID that we can cancel, in the same way that the delay function

b-viguier · 2020-01-02T15:22:40Z

src/Adapter/Amp/EventLoop.php

@@ -250,7 +301,9 @@ function ($watcherId, $stream) use ($deferred) {
            }
        );

-        return Internal\PromiseWrapper::createUnhandled($deferred->promise(), $this->unhandledFailingPromises);
+        $cancellation = function () {};


Same remark: onWritable returns a watcher ID that we can cancel, in the same way that the delay function

b-viguier · 2020-01-02T15:58:49Z

src/Adapter/Amp/Internal/PromiseWrapper.php

-    public static function createUnhandled(\Amp\Promise $ampPromise, FailingPromiseCollection $failingPromiseCollection)
+    public function cancel(): void
+    {
+        ($this->cancellation)();


public function cancel(string $reason): void { ($this->cancellation)($reason); }

b-viguier · 2020-01-02T16:45:55Z

src/Adapter/ReactPhp/EventLoop.php

+            function () use (&$deferred) {
+                $deferred->reject(new \M6Web\Tornado\CancelledException());
+            }
+        );


Looks strange… this code means that when the promise of a deferred is cancelled you want to reject the deferred to… reject the promise that you cancelled 🤨
According to the documentation, it's not possible to cancel futureTick in ReactPhp… to ensure consistent behavior, we could use a timer with a 0 delay 🤔

b-viguier · 2020-01-02T16:46:33Z

src/Adapter/ReactPhp/EventLoop.php

@@ -208,10 +235,10 @@ function () use ($deferred) {
    /**
     * {@inheritdoc}
     */
-    public function deferred(): Deferred
+    public function deferred(callable $canceller = null): Deferred


At the end, all $canceller parameters must be mandatory

b-viguier

Internal Tornado implementation review

b-viguier · 2020-01-03T08:02:04Z