Hello, Rob! It is probably best to have channels be non-buffered by default (like in > Go); buffered channels can hide architectural issues in async code. > Great point! If a channel has a default capacity greater than 1, it leads to implicit behavior that the user must be aware of. If something results in implicit behavior, it's a bad practice. I'll fix it!
> 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? Because additional methods allow us to express explicit semantics while also performing extra checks. Although, in essence, it's the same as the close() method. I'm not 100% sure whether this implementation is the right choice because it has more elements compared to Go. However, the close() method has an ambiguity regarding whether we want to terminate the Consumer or the Producer. And ambiguity leads to errors. > Personally, it should be an exception (or at least a warning) if a channel gets GC'd with pending data. Yes, we need to think about what would be better—an exception or a warning. The GC considers whether the channel was explicitly closed or not. However, if someone tries to close consumption while there is still data, a warning is mandatory because this is, once again, *explicit* semantics. If a programmer wants to close a channel with data, they must first call discardData() and only then close it. So yes, explicit operations are much better. > 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. What exactly is difficult to use? > Can we name this Async\Closure or something similar? Just to keep it inline with the current \Closure It's hard for me to judge this. On one hand, the name *Callback* clearly reflects what this object does. > Speaking of, you probably meant to use \Closure as the $callback property instead of mixed? Unfortunately, PHP does not support the *callable* type for properties. > - The constructor for a Future is public, but it looks like it would be weird to use. I'm currently working on this module and decided to base it on the implementation in AMPHP. So, the Future class will just be a wrapper around FutureState. And yes, its constructor can remain public, allowing Future to be used as DeferredFuture. > It would be nice to see this simply use Futures as much as possible. For example, returning a Future instead of FiberHandles. Yes, this will be implemented a bit later, after the *Future* module is fully ready. Most likely, this week. > 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. Yes, these are OS signals, but they are essentially cross-platform. Can int be replaced with something else, like an enum? Of course. > - Exposing $reserved -- which is accessible via reflection -- is probably a bad idea. Yes, I’ve thought about this. It really doesn’t seem like the best idea. I might end up making the Notifier object final, which would eliminate this issue. > 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. Yes, all Notifier classes have a string representation for analysis purposes. Do you think it would be better to use a separate function instead of __toString to avoid implicit behavior? If it helps prevent unintended behavior, then you’re probably right. > Instead of Terminate(), should it be Close(), to be consistent with the rest of the library? Yes, maybe close() is better. > - It would be nice to get a list of callbacks on the Notifier as well. I 100% agree. I'll add this method now before I forget. > It would be nice to be able to get a future from the Walker instead of polling "isFinished". Of course. > 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: Sorry, but for some reason, I don't see the code. But just in case, I can say in advance that Walker was essentially made thanks to *AMPHP*—I borrowed part of the implementation from there :) > 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. Why do you need Walker for this? > - 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). This looks more like AwaitAll() / AwaitAny(). These functions will also be available once I finish Future. > $result = match (Async\select($channel)) { > Async\EOF => "channel closed", > default => $channel->receive(), >} Why? foreach ($channel as $data) { // ... } just use it :) > Also, keep in mind this is pretty valid That's exactly how it's implemented now. The channel does not treat NULL as the end of transmission—the programmer must close it explicitly, or the *GC* will do it. Thanks for the great comments! They will help make this product better.
