Hi, On Sun, Mar 8, 2026 at 12:27 PM Tim Düsterhus <[email protected]> wrote:
> Hi > > Am 2026-02-22 17:16, schrieb Jakub Zelenka: > > Thanks for the feedback! > > Thank you. I've given the RFC another read now and have some minor > things to remark: > > 1. I don't think it makes sense for the majority of the classes to be > serialized, because they rely on “in-process” state, specifically file > descriptors. This includes Context, Watcher, and the handles. > Serializing the enum and Exceptions by themselves is fine. The RFC > should note that serialization is prevented and the implementation > adjusted. > > Sure this was my omission - I marked all classes as not serializable. > 2. Similarly for any new classes, I recommend to always set > `@strict-properties` to prevent shenanigans with folks accidentally > adding dynamic properties to these classes. This should be done for > everything, including the exceptions. > > And I also marked them with @strict-properties > 3. I have a question as someone who never used Fibers in production: Is > it necessary or helpful for the API to provide some “convenience” > functionality related to Fibers (e.g. automatically suspending or > unsuspending Fibers)? I'm asking now, because adding that functionality > later might be a breaking change in behavior. If it's not necessary or > can be added in a backwards compatible way, that's fine, of course. > > I can't really think about something reasonable here - it seems better not to mix it. And even if we wanted to add something, I think it could be backward compatible. > > On Sun, Jan 18, 2026 at 1:24 PM Tim Düsterhus <[email protected]> wrote: > >> - Context::wait() takes a millisecond timeout. This feels > >> insufficiently > >> granular. I am wondering if we should rather use either micro (as > >> returned by microtime) or nanoseconds (as returned by hrtime) - or > >> possible an instance of DateInterval (which would use microsecond > >> resolution). > >> > > > > Yeah this makes sense. I used the same convention that is used in > > streams > > which means two params so it is changed to: > > > > public function wait(int $timeoutSeconds = -1, int $timeoutMicroseconds > > = > > 0, int $maxEvents = -1): array {} > > I feel that using two parameters is providing for a very clunky API, > which is also acknowledged by the doc comment on $timeoutMicroseconds > "Only used when $timeoutSeconds >= 0". I would have suggested > DateInterval again, but I'm just seeing that it seems there is no > documented way to manually construct a DateInterval with microseconds. > > Perhaps the following suggestion would be a good middleground: > > - Change $timeoutSeconds to int|null: If null is specified, the timeout > is infinite. > - Throw ValueError if $timeoutSeconds < 0 > - Throw ValueError if $timeoutMicroseconds < 0 > - Throw ValueError if $timeoutSeconds === null && timeoutMicroseconds > > 0 > > `null` is a much more natural value for “no timeout” and by throwing > ValueError we make sure that we don't have a situation where a parameter > is silently ignored. > Yeah this makes much more sense and stream_select works similar - not sure why I didn't add it. It's fixed. > > Similarly for `$maxEvents` it might make sense to use `null` for “use a > default value”? Also: What happens if I specify `$maxEvents = 0`? Is > this useful? > 0 was used to select default so the same as -1. I changed it to null and it now has to be positive value. > > > > >> - For Handle I am wondering about abstract class vs interface. Since > >> the > >> Handle presumably doesn't have any non-abstract methods, an interface > >> feels more correct / more flexible. > >> > > > > I have been considering this already and there are reasons why I prefer > > abstract method here. > > > > The abstract method is better for internal implementation and limit > > overhead of PHP calls that would be required for interface. The > > advantage > > is that I can use internal struct API through C operations which allows > > bypasing the call and can do optimization for some backends - e.g. > > timeouts > > for kqueue don't need file descriptor. > > You can bypass the “VM method” for interfaces and abstract methods > alike. A good example would be ext/random which allows changing the > engines by the Random\Engine interface, but for engines implemented in > C, the Randomizer doesn't go through the userland API, but instead calls > the engines directly. > > Yeah I think perf would be solvable here actually. > > I also think that there is not really much use case for user space to > > implement their own handles so such interface would be used only > > internally > > anyway. > > This applies equally to interfaces and abstract methods. The abstract > base class however will make it much weirder when a specific (future) > handle might need to implement additional interfaces or abstract > classes. > > > In addition interface would effectively expose the internal stream fd > > which > > is currently hidden and makes harder messing up with stream fd which > > might > > cause various issues. > > I don't understand that point. For both an interface and an abstract > method, the method would exist on the child class and thus can be called > by a developer. > > Well if it is abstract, then the method can be protected and because the classes are final, user spaces cannot call it. But for interface I would need to make it public which means that StreamHandle would need to expose callabable (public) method. I know that I could just return 0 and use different handling internally but I think this would be surprising and created obvious inconsistency. I mean it's fine if the calls happen internally but if the exposed methods are just dummy and return nonsense for user space, then I don't think it would be a good design. Kind regards, Jakub
