Hi, Thanks for the feedback!
On Sun, Jan 18, 2026 at 1:24 PM Tim Düsterhus <[email protected]> wrote: > Hi > > On 10/30/25 22:06, Jakub Zelenka wrote: > > I would like to introduce a new polling API RFC that is part of my stream > > evolution work: > > > > https://wiki.php.net/rfc/poll_api > > Since you made some larger changes to the RFC, rather than checking each > change individually, I've taken another full look at the RFC and have > the following remarks: > > - 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 {} > - For Handle: I assume that getFileDescriptor() should be abstract? > I actually thought that it might be nicer to have default method for internal classes but that wouldn't work well for user classes anyway so it's better to make it abstract which I just did. > - 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. 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. 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. > - For Handle: What happens if I extend this class in userland and attach > it to a Context. Will things break? What if I return the number of a > non-existent FD? > Depending on backend it will result either in FailedHandleAddException (epoll, kqueue, possible event ports as well) or on Windows it would likely fail during wait with FailedPollWaitException. So it can be handled by catching FailedPollOperationException. This is however very unlikely edge case so I don't think there is much point to worry about consistency. As I said the use case for implementing custom Handle is a bit moot. > - For the stubs: It would be useful if you used the “generics notation” > for the array returns. e.g. `@return list<Backend>` for > Backend::getAvailableBackends(). This makes it easier to understand how > exactly the result will look like to check for mistakes / suboptimal > choices. > Makes sense. I added it also for Watcher::getTriggeredEvents, Watcher::getWatchedEvents that return list<Event> and Context::wait that returns list<Watcher>. > - InactiveWatcherException: This one feels more like a programmer error, > so should possibly be a PollError / InactiveWatcherError (but I don't > know enough about the topic to be sure). > I don't think it should be error as it is not necessary for user to track removal of watcher (which can also happen automatically for one shot events). It should be just fine to normally recover from this rather than requiring checking for isActive. It's more state issue which is usually handled using exceptions IMHO. > - Internal API: For php_poll_wait(), the timeout should probably be a > struct timespec for future-proofing. > > This has been changed as suggested (together with introducing microseconds support for user space). In addition to the above changes I also changed ValueError for passing backend that is not available on the current platform to new BackendUnavailableException as this is more runtime specific issue that application might want to handle rather than forcing users to always use Backend::isAvailable. Basically I feel if something can vary and it's not possible to be immediately caught by developer (because they can develop it on different platform), then it should be more an exception than error. Kind regards, Jakub
