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

[Feedback] Welcome to RevoltPhp 🥳 #1

Closed
b-viguier opened this issue Oct 15, 2021 · 7 comments
Closed

[Feedback] Welcome to RevoltPhp 🥳 #1

b-viguier opened this issue Oct 15, 2021 · 7 comments

Comments

@b-viguier
Copy link

Hi there 👋 😄 ,
I'm Benoit, an enthusiast Php/Async dev, and I just wanted to give my feedback (+ questions) as proposed in this tweet.

First of all, thank you very much for all this work! Even if I have some comments about RevoltPhp design, I'm really convinced that it's an awesome opportunity for Php ecosystem to see ReactPhp & Amp teams (& friends) working together to build such library 👍 🤩

Some context

When I had to choose an EventLoop for my company internal project, I studied a lot ReactPhp and Amp developer experience (DX). I really like the EventLoop Interface in ReactPhp, but I was really concerned by the potential callback hell for my team. In the other hand, Amp use generators with a nice DX (more intuitive to read IMHO), but EventLoop is global and doesn't expose an interface (but driver does). That's why we created Tornado, a modest attempt to abstract both with the Generator approach… So, when I read the first tweet of RevoltPhp I was very curious 😄

Fibers

I'm really excited about Fibers, I think it's the perfect tool to provide a fluent DX for async processing. But surprisingly, RevoltPhp seems to use callbacks extensively… Do you consider RevoltPhp like a very low-level async layer on which we can build some Fiber-oriented libs/projects?
Nevertheless, it seems possible to rely on Fiber with the Suspension concept (example), but it seems very verbose.

$watcher = EventLoop::onWritable($stream, fn () => $suspension->resume(null));
$suspension->suspend();
EventLoop::cancel($watcher);

I was maybe expecting something like this:

EventLoop::waitWritable($stream);

Or maybe keeping the concept of Promise/Future, with something like:

$foo = EventLoop::wait(EventLoop::async(foo(...)));

🤷
Anyway, all these functions can be created from Revolt core functions, it's just to give you my (humble) opinion 😃

Combinators

Do you intend to provide some basic functions to combine asynchronous functions? Like all, race… ?
Once again, it's totally possible thanks to suspension (example), but it's not straightforward IMHO

Conclusion

That's all 😄 Of course, I would prefer to have a non-global EventLoop or an EventLoopInterface (instead of using Drivers), but I don't think it's the most important, just a matter of taste 😉
And once again, thank you for this library 👍 Even with this design, I'm pretty sure it will be very useful as a core component to other projects.
If you are curious, I created a 1-file async-library for an incoming talk, to quickly show possibilities offered by Fibers: https://github.com/b-viguier/Slip . Just for demonstration purpose, but feel free to comment 🤷

Benoit

@kelunik
Copy link
Member

kelunik commented Oct 15, 2021

Hey Benoit,

welcome to Revolt! 👋 You're raising some good points, which I'll try to address below.

Revolt is indeed planned to be a very low-level core component. It provides what's necessary to build interoperable concurrent libraries, but not much more.

We've discussed different approaches such as adding something like EventLoop::waitWritable($stream);. While such an API works great for simpler use cases, it falls short of supporting more complex use cases, such as timeouts or cancellations of the operation. There are (of course) various ways to address these shortcomings, but it's likely to forget specific use cases. Thus, we have decided to go with callbacks at the lowest level, as we have years of experience with this approach, and it allows building an waitWritable API at a higher level on top of the callback API, but not the other way around.

Using the suspension API for a single read is indeed quite verbose. The plan is to have libraries be built on top of the low-level APIs Revolt provides, like we've been doing for years, see amphp/byte-stream and react/stream. There are different approaches to stream abstractions that all have their own trade-offs, so we won't provide any such abstraction as part of the revoltphp organization for now. We expect that most users will use these abstractions instead of directly registering event callbacks using the Revolt APIs.

As mentioned on Twitter, we currently don't ship an async placeholder / promise / future implementation. Because of that, there also won't be any combinators for now. Amp and ReactPHP will ship different implementations for now, but we might agree on a shared implementation in the future.

The Driver interface is exactly the non-global EventLoopInterface you're looking for. Revolt\EventLoop simply forwards the calls to the appropriate driver. As there can only be one event loop in each application, it makes sense to make it global. Amp and ReactPHP already ship with such a global facade. Consider the event loop being part of the runtime. In JS nobody worries about setTimeout being global either.

I'm happy to answer more questions if you have any. 😄

@trowski
Copy link
Member

trowski commented Oct 15, 2021

To expand on what @kelunik mentioned about placeholders / promises / futures, different implementations of such objects can be interoperable as long as they use the same event loop to schedule resolution. So there's not really a need to standardize that right now.

An await function for ReactPHP's promise could be implemented as:

function await(PromiseInterface $promise): mixed {
    $suspension = EventLoop::createSuspension();
    $promise->done(
        fn ($value) => $suspension->resume($value),
        fn ($exception) => $suspension->throw($exception)
    );
    return $suspension->suspend();
}

An actual implementation would likely be more complex to provide other options.

amphp v3 uses a new placeholder, Future, and will provide a set of combinators for futures. The implementation is rather opinionated, depending on amphp specific tools, such as providing the ability to cancel awaiting using Amp's cancellation tokens.

amphp v3 provides a delay function that is a wrapper around an EventLoop::delay() watcher plus providing the ability to cancel. Again though, this is an opinionated implementation in the same way as futures above.

Hopefully this shows how quickly seemingly simple functionality can become subjective. That is why we are providing only the minimal tools necessary to allow building various APIs on top of Revolt.

@WyriHaximus
Copy link
Contributor

I really like the EventLoop Interface in ReactPhp, but I was really concerned by the potential callback hell for my team.

Hey Benoit, as a ReactPHP core developer I totally understand that concern. It has taken me some time to get used to that and not to over do it (because it is so easy to over do it) by designing and chaining promises well. The Fibers RFC made my very excited because it gives us the good parts from Generators but not the bad parts. So the possibility to create more sync looking code without the callbacks and the drawbacks of Generators is a very welcome possibility.

What we are looking at with the ReactPHP team is not only how to create that await function @trowski showed, but also how and when to use fibers instead of promises so we don't have to do an await function when we don't have to. But we also could provide API's that expose both, we're still in flux on how our API's will look like.

@b-viguier
Copy link
Author

Many thanks for these accurate replies, it's clearer now 😄 Proceeding step by step is a smart move 👍

And @trowski is right, as soon as you introduce some higher level concepts you also introduce some strong opinions/choices… it's maybe too early for that. I was expecting the unique async framework to rule them all, but now I realize how ambitious it is 😉

In this current state, I think that RevoltPhp EventLoop is dedicated to async-aware developers (and it's fine!), it's a very good candidate to be the common foundation of many higher level libraries for a larger audience.

Suspension class looks also very interesting, I definitely have to dig further into this 😅

Now I'm looking forward to see what impact it will have on React & Amp 😉

@trowski
Copy link
Member

trowski commented Oct 15, 2021

I was expecting the unique async framework to rule them all

Quite the opposite, as you've seen. 😁

Revolt's goal is to provide what is essentially a runtime upon which any async package or framework can be built. Think of it like JavaScript's event loop. Users may mix and match async packages that use Revolt, rather than be committed to packages from only a single framework due to dependence on a particular event loop.

@kelunik
Copy link
Member

kelunik commented Oct 24, 2021

@b-viguier I've clarified in the README now that Revolt won't be a full-blown framework, but rather only the common base for other libraries to be built on top.

@b-viguier
Copy link
Author

Yeah, that's very clear now according to the documentation 👍
Thanks again for all of this 🎉
I'm closing this issue since I have nothing to add 🤷

digitaltim-de added a commit to digitaltim-de/event-loop that referenced this issue Jan 26, 2025
Fatal error: Uncaught FiberError: Cannot suspend in a force-closed fiber in /srv/app/vendor/revolt/event-loop/src/EventLoop/Internal/AbstractDriver.php:441 Stack trace: #0 /srv/app/vendor/revolt/event-loop/src/EventLoop/Internal/AbstractDriver.php(441): Fiber::suspend(Object(stdClass)) revoltphp#1 /srv/app/vendor/revolt/event-loop/src/EventLoop/Internal/AbstractDriver.php(567): Revolt\EventLoop\Internal\AbstractDriver->invokeMicrotasks() revoltphp#2 [internal function]: Revolt\EventLoop\Internal\AbstractDriver->Revolt\EventLoop\Internal\{closure}() revoltphp#3 {main} thrown in /srv/app/vendor/revolt/event-loop/src/EventLoop/Internal/AbstractDriver.php on line 441
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants