amphp / amp

A non-blocking concurrency framework for PHP applications. 🐘
https://amphp.org/amp
MIT License
4.25k stars 257 forks source link

TimeoutCancellation. Cannot understand how it work #444

Closed mDistefanoFacileIt closed 1 month ago

mDistefanoFacileIt commented 1 month ago

Hello, i have a very simple use case where i would like to use TimeoutCancellation but can't undestand how it exactly works. I have two Future, and i'm running them with awaitAll. I would like to cancel one when a timeout is reached but still keep the result from the others.

I tried to use TimeoutCancellation like following but then a CancelledException is raised

        $f1 = async(function () {
            delay(10);
            echo 'F1 completed' . PHP_EOL;
        });

        $f2 = async(function () {
            delay(5);
            echo 'F2 completed' . PHP_EOL;
        });

       awaitAll([$f1, $f2], new TimeoutCancellation(7));

So, i looked into awaitAll and tried to move the TimeoutCancellation into the foreach but it seems to be ignored:

        foreach (Future::iterate([$f1, $f2]) as $index => $future) {
            try {
                $values[$index] = $future->await(new TimeoutCancellation(7));
            } catch (\Throwable $throwable) {
                $errors[$index] = $throwable;
            }
        }

So i tried to remove the Future::iterate, this seem to work but i'm not sure if this is correct:

        foreach ([$f1, $f2] as $index => $future) {
            try {
                $values[$index] = $future->await(new TimeoutCancellation(7));
            } catch (\Throwable $throwable) {
                $errors[$index] = $throwable;
            }
        }

I'm afraid to loose something not using the Future::iterate. It's possible to do what i want? Or I'm not understanding its purpose?

Any suggestion is welcome. Thank You

xpader commented 1 month ago

Maybe you should use awaitAny.

        $f1 = async(function () {
            delay(10);
            echo 'F1 completed' . PHP_EOL;
        });

        $f2 = async(function () {
            delay(5);
            echo 'F2 completed' . PHP_EOL;
        });

       awaitAny([$f1, $f2], new TimeoutCancellation(7));
mDistefanoFacileIt commented 1 month ago

@xpader from what i can understand, awaitAny return the result of the first Future completed. I would like:

  1. If all Future are completed i have a result for all of them.
  2. If one or more Future exceed a timeout, i have the result only for Future completed.
kelunik commented 1 month ago

So, i looked into awaitAll and tried to move the TimeoutCancellation into the foreach but it seems to be ignored:

What do you mean with ignored? One possible issue here is that you create a new cancellation for every await, so if your first completion takes 3 seconds, the actual timeout for the second one will be 10 seconds instead of 7 seconds.

You can fix that by reusing the cancellation, which is more efficient anyway:

$cancellation = new TimeoutCancellation(7);

foreach (Future::iterate([$f1, $f2]) as $index => $future) {
    try {
        $values[$index] = $future->await($cancellation);
    } catch (\Throwable $throwable) {
        $errors[$index] = $throwable;
    }
}

So i tried to remove the Future::iterate, this seem to work but i'm not sure if this is correct:

I'm afraid to loose something not using the Future::iterate. It's possible to do what i want? Or I'm not understanding its purpose?

Future::iterate ensures futures are awaited in completion order here. If you don't need that and just want everything completed within the timeout (in any order), then you don't need it.

mDistefanoFacileIt commented 1 month ago

What do you mean with ignored? One possible issue here is that you create a new cancellation for every await, so if your first completion takes 3 seconds, the actual timeout for the second one will be 10 seconds instead of 7 seconds.

You can fix that by reusing the cancellation, which is more efficient anyway

Hello @kelunik tnx for your reply. I dont know if i'm missing something here. I created a small test with PHPUnit. I created the TimeoutCancellation outside the loop with a timeout of 1 second, i'm expecting both of my Future go timeout and been in the error array's. But both are in the values and the test complete in 10 second.

<?php

use Amp\Future;
use Amp\TimeoutCancellation;
use PHPUnit\Framework\TestCase;
use function Amp\async;
use function Amp\delay;

class CancellationTest extends TestCase
{
    public function testCancellation(): void
    {
        echo PHP_EOL;

        $f1 = async(function () {
            delay(10);
            echo 'F1 completed' . PHP_EOL;
        });

        $f2 = async(function () {
            delay(5);
            echo 'F2 completed' . PHP_EOL;
        });

        $values = [];
        $errors = [];

        $cancellation = new TimeoutCancellation(1);
        foreach (Future::iterate([$f1, $f2]) as $index => $future) {
            try {
                $values[$index] = $future->await($cancellation);
            } catch (\Throwable $throwable) {
                $errors[$index] = $throwable;
            }
        }

        self::assertCount(2, $errors);
        self::assertCount(0, $values);
    }
}

This is my stdout:

F2 completed
F1 completed

Time: 00:10.016, Memory: 14.00 MB

I'm using version * v3.0.2 under PHP8.1

kelunik commented 1 month ago

Oh, yes, indeed, it won't work that way. Future::iterate already waits for completion internally before "submitting" the futures to the iterator, so at the point of "await" the future is already completed, so the timeout has no effect anymore. So you need to indeed skip Future::iterate().

mDistefanoFacileIt commented 1 month ago

@kelunik thank you. Now i understand a bit better. I think i will try to skip Future::iterate() and use a simple array like this:

        $cancellation = new TimeoutCancellation(15);
        foreach ([$f1, $f2] as $index => $future) {
            try {
                $values[$index] = $future->await($cancellation);
            } catch (\Throwable $throwable) {
                $errors[$index] = $throwable;
            }
        }

I think we can close