revoltphp / event-loop

Revolt is a rock-solid event loop for concurrent PHP applications.
MIT License
807 stars 28 forks source link

Feature Request: Provide a way to find out if a calbackId is valid #91

Open Kingdutch opened 7 months ago

Kingdutch commented 7 months ago

Problem Description

I want to run code like the following:

// Perform eager tasks as often as possible but never more than one at a time.
$callbackId = EventLoop::repeat(0, function ($callbackId) {
  EventLoop::disable($callbackId);
  $this->performHalfSecondTask();
  EventLoop::enable($callbackId);
});

// Simulate some long task that mostly suspends.
EventLoop::delay(1.25, function() use ($callbackId) {
  echo "Cancelling repeat\n";
  EventLoop::cancel($callbackId);
});

EventLoop::run();

The problem with this is that there's no efficient way to check if $callbackId is still valid when running EventLoop::enable($callbackId); which means that there's a guaranteed exception somewhere in the program.

Workarounds

There's two possible ways around this, neither of which are great:

1) Check all the identifiers, this is potentially expensive if the list of identifiers is large due to a high number of async tasks.

if (in_array($callbackId, EventLoop::getIdentifiers, TRUE)) {}

2) Catch the exception. This feels quite verbose and it's unclear whether this is intentional or whether a developer unintentionally hides a bug here.

try {
    EventLoop::enable($callbackId);
}
catch (EventLoop\InvalidCallbackError $e) {}

Feature Request

It would be great if there is a method saying EventLoop::isValidIdentifier which does an isset behind the scenes.

if (EventLoop::isValidIdentifier($callbackId)) {
   EventLoop::enable($callbackId);
}

Alternatively a second parameter to enable/disable to indicate it may fail silently would work too:

EventLoop::enable($callbackId, TRUE);
kelunik commented 7 months ago

What is $this->performHalfSecondTask() doing? repeat(0) is a good indicator that there's something solved the wrong way and better options exist.

Kingdutch commented 7 months ago

In this case performHalfSecondTask is pre-warming one of N caches in an arbitrary order. It's essentially an optional background task: if we're waiting for some file or network I/O, pre-warm the cache to speed up future work, but otherwise just stick with what we're doing.

If our main task (handling a web request) is complete then we want to stop pre-warming caches and end the request.

Kingdutch commented 7 months ago

I've pushed a more complete example in https://github.com/Kingdutch/revolt-playground/commit/528a931a1c321751b3babfb7586c1c357ba5079f :)

trowski commented 7 months ago

One suggestion is to use a reference in the closures to set a flag that the callback has been cancelled.

$cancelled = false;
$callbackId = EventLoop::repeat(0, function (string $callbackId) use (&$cancelled): void {
    EventLoop::disable($callbackId);
    $this->performHalfSecondTask();
    if (!$cancelled) {
        EventLoop::enable($callbackId);
    }
});

EventLoop::delay(1.25, function () use (&$cancelled, $callbackId): void {
    EventLoop::cancel($callbackId);
    $cancelled = true;
});

If you use object properties instead you can completely avoid the by-reference use, but you will be creating a circular reference to the object. This may or may not matter for your application.

As @kelunik pointed out, this is a bit of an odd use of event-loop callbacks. Other async abstractions would be useful here, such as Promises/Futures, cancellations, or async iterators (pipelines as we call them in amphp/pipeline).