On Fri, Feb 14, 2025, at 15:16, Edmond Dantes wrote: > Hello, *Jakub.* > (It seems that simply clicking "Reply" does not work for the conference) > ** > *> *I understand that's not ready for review yet but quickly > You could say that the code is in a state of "architectural stability," so > it can be reviewed and criticized. (Unfortunately, only the Windows build is > currently working.) > In the near future, I plan to start documenting the C-API, where I will > describe the implementation details in depth, but feel free to ask any > questions about this component. > > Link: https://github.com/EdmondDantes/php-src/tree/async/async >
Just looking through the PHP stubs, and here are some notes: Channels: - It is probably best to have channels be non-buffered by default (like in Go); buffered channels can hide architectural issues in async code. - Why do we need "finishing" producing and consuming? Won't "closing" the channel be enough, or does closing a channel prevent consumers from reading pending items? Personally, it should be an exception (or at least a warning) if a channel gets GC'd with pending data. - Looking at the public api, it seems that it is very hard to use correctly. It would probably be a good idea to pair it down and come up with an api/ideomatic usage that makes it hard to use incorrectly. Callback: - Can we name this Async\Closure or something similar? Just to keep it inline with the current \Closure - Speaking of, you probably meant to use \Closure as the $callback property instead of mixed? Futures: - The constructor for a Future is public, but it looks like it would be weird to use. Module: - It would be nice to see this simply use Futures as much as possible. For example, returning a Future instead of FiberHandles. - Is onSignal OS signals or application signals? (I didn't look at the C code) If the latter, it is probably better to use something other than integer. If the former, this is usually handled by extensions (such as pcntl) and comes with a ton of caveats. It might be better to let those extensions handle that. Notifier: - Exposing $reserved -- which is accessible via reflection -- is probably a bad idea. - Should this be implementing Stringable interface? Also, this will automatically cast to a string if accidentally passed to a function that takes a string -- unless you are using strict mode. https://3v4l.org/3AqcW This is probably not desired. - Instead of Terminate(), should it be Close(), to be consistent with the rest of the library? - It would be nice to get a list of callbacks on the Notifier as well. Walker: - It would be nice to be able to get a future from the Walker instead of polling "isFinished". - IMHO, it would be better to implement this around Futures (see amphp's extensive library around this). Here are some examples from my own code: - timeouts: race two futures, one which times out and the other which performs a long-running task. If the timeout wins, then I cancel the long-running task and proceed as an error. If the long-running task wins, I cancel the timeout. - wait for all to complete: there are a couple of variations on this theme. But sometimes we want to wait for all to complete, errors or not; sometimes we want to stop on the first error; and sometimes we want to just get the first success (see timeouts). All in all, this is a good start. It would probably be a good idea to take a deeper look at other Channel implementations and borrow heavily from them. The current implementation seems to leak a lot of internal information that most programs should not use. As a producer, all I care about is that the channel is open; not that anyone is listening or not. There may be nobody listening right now; but because it is async, it is my job as a programmer to make sure that someone is listening, eventually. Same with consumers, they don't need to know whether someone is actually producing, just that the channel is open so they can consume. It may be a good idea to create a way to use it with match: $result = match (Async\select($channel)) { Async\EOF => "channel closed", default => $channel->receive(), } Where Async\select() just pauses until there is a value and returns "null" or "EOF" or something when it has a value available or becomes closed. Also, keep in mind this is pretty valid: $channel->send($data); $channel->close(); A consumer should read $data before seeing the channel as closed. I presume that this is what "finishProducing()" is for, but it requires synchronization to work properly and the entire idea of channels is to not need synchronization at all. — Rob
