Re: [RFC] Polling API

On Fri, Oct 31, 2025 at 1:06 PM Jakub Zelenka <bukka@php.net> wrote: > Hi, > > On Fri, Oct 31, 2025 at 10:40 AM Tim Düsterhus <tim@bastelstu.be> wrote: > >> Hi >> >> Am 2025-10-30 22:06, schrieb Jakub Zelenka: >> > 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 >> >> 1. >> >> Thank you for the RFC. I've taken a first skim of the proposal and it >> immediately raised the question of naming and namespacing in particular. >> Our naming policy at >> >> https://github.com/php/policies/blob/main/coding-standards-and-naming.rst#bundled-ewill >> be alwaysxtensions >> >> <https://github.com/php/policies/blob/main/coding-standards-and-naming.rst#bundled-extensions> >> says that “namespaces SHOULD be used” and given that this is a >> completely new API, I think we should namespace them. >> >> My understanding is that the proposed API relies on a file descriptor >> and not something like a timeout. It therefore makes sense to me to put >> it into a namespace Io\Poll; or similar. We would then also >> have: >> >> namespace Io; >> class IoException extends \Exception {} >> namespace Io\Poll; >> class PollException extends \Io\IoException {} >> >> > I thought about this and think this might be a good idea. > > This is implemented as suggested and RFC updated. > >> The StreamPollHandle method should possibly be placed in the global >> namespace still, since the stream functions are sitting there - and a >> SocketPollHandle would of course be sitting in the namespace of the >> Sockets extension. >> > > Yeah those could stay in global and just extend Io\Poll\Handle. > > >> 2. As for the PollBackend enum. >> >> Is there a reason why this is a backed enum? Generally speaking enums >> should not be backed, unless there is a good reason for this. I don't >> think there is in this case - and none of the native enums are backed so >> far. >> >> > I missed that they should not be backed. I just saw `enum AdjacentPosition > : string` in Dom so thought that it's fine to use it... Will change it. > > This is also fixed and it's no longer a backed enum. > 3. Exception-wise. >> >> StreamPollHandle::__construct(): This should probably be a ValueError, >> not a PollException, since passing an invalid stream is a programmer >> error. >> > > This makes sense. > > >> >> In the other cases it probably makes sense to further split the >> PollException into purpose-built exceptions according to the Throwable >> policy at >> >> >> https://github.com/php/policies/blob/main/coding-standards-and-naming.rst#throwables >> (“The exception message MUST NOT be the only means of distinguishing >> exception causes that the user might want to handle differently.”). >> >> > This makes sense and I will introduce more exceptions. I will not use > exception per errno but some middle ground is a good idea. Maybe per op > exception with codes representing specific errors would be make sense? > > I created a new exception hierarchy that is hopefully in line with Throwable policy. > As an example PollContext::__construct() should probably throw a >> BackendUnavailableException or something like this. > > There are now getAvailableBackends so users should check it out before so I used ValueError for this part. > >> 4. PollBackend >> >> Is the availability of the backends known at compile time of PHP or at >> runtime only? Depending on that it might make sense to only >> conditionally define the enum cases, allowing users to check >> availability with defined() or checking the output of >> ::cases(). >> Alternatively, a public static function getAvailableBackends(): >> array >> could be added. >> >> > It's compile time but not sure if I like exposing enum only if compiled in > as it makes the checks harder. This part is not really something that > users should use but it's really more for testing purpose. In reality > everyone should just use default Auto... But getAvailableBackends() might > make sense even for testing. Maybe it could also have per enum method > isAvailable() so user can check that but not sure if it's needed. > > Added getAvailableBackends(), isAvailable() and supportsEdgeTriggering() methods. Kind regards, Jakub