Poll selector

[Janrupf]

trafficstars

This PR adds support for using the Unix poll API as a backend.

Why this is required

There are some operating systems out there which only support poll (or other API's). The main challenge here was to track the file descriptors, as the poll syscall requires all file descriptors at call site. This is solved by keeping a mutex around the state of the selector and thus tracking everything added/removed from it.

The main motivation for this PR is to enable support for the esp-idf and it's growing eco system, eventually allowing tokio to fully run on these devices. As the esp-idf only supports poll, mio (and by this extent the full feature set of tokio) is unable to run on it. Additionally this benefits OS such as Haiku (see https://github.com/tokio-rs/mio/issues/1472), which also only provide a poll based API.

Does this work?

The short answer is: Yes, it should.

The long answer: the polling crate already uses this, and in fact, the implementation here is largely a copy-and-paste of the one provided by polling. There are some notable differences whatsoever:

• mio additionally needs cloning support, this is achieved by keeping everything in an Arc
• mio (and tokio) used to assume edge triggering, but poll is level triggered (may be a breaking change!)

What did change

In addition to a new a new polling backend, a feature called force-old-poll was added in order to allow further testing of the poll implementation (this feature is required to force use of the poll API, whenever the OS provides kqueue or epoll). It also now really is necessary to deregister event sources, else stale events may be fired (the general rules seems to always have been do deregister event sources, tokio's PollEvented for example does so as well as all mio examples).

The mio Waker also gained a new method called did_wake, which needs to be called after a task has been woken up in order to reset the event source (and not wake the task again when polling). This requires a very small patch in tokio which I have ready.

Effect on downstream crates

Probably none, but this is still a breaking change (see the notes about Waker and the edge vs. level triggered situation).

Since the poll implementation will virtually not be used anywhere other than new systems, no existing code should break on existing systems. Still, the API did change and does have new requirements. I expect most downstream crates to properly deregister event sources before destruction, but this now is absolutely required to not run into weird race conditions. Additionally, every crate using mio Wakers now needs to call did_wake after being woken by the Waker.

Notes

This needs a lot of testing and feedback from the tokio and mio maintainers. Also thanks to @Kestrer, who implemented large portions of this code for the polling crate.

[Thomasdezeeuw]

Before starting to work on large things like this, please open an issue first explain why you need this.

[Janrupf]

This started out as an experiment to see if it even was possible - turns out, it is and works pretty well. I thought I'd PR that for a review - that being said, if you still want me to open an issue I can do so (though I guess that would mostly be a copy'n paste of the PR description)

[Janrupf]

I have to wonder: should we also support the select() libc function, for the systems that don't comply with POSIX?

I thought about this as well, my reasoning for not using select directly was its limitation of 1024 fd's. A Selector using select would look mostly the same as this one.

Fun fact: the platform I'm specifically targetting with this PR implements poll using select and just starts screaming when you pass in too many file descriptors (see https://github.com/espressif/esp-idf/blob/master/components/newlib/poll.c#L13). For a proper Unix system using poll is still better than select.

So if you need this right now, you can also just implement poll this way :smile:

[Thomasdezeeuw]

This started out as an experiment to see if it even was possible - turns out, it is and works pretty well. I thought I'd PR that for a review - that being said, if you still want me to open an issue I can do so (though I guess that would mostly be a copy'n paste of the PR description)

Yes please open an issue as we'll mostly need to figure out non-code stuff. Things such as who's going to maintain this, CI, OS targets, etc.

[Janrupf]

Also just mentioned a couple of time you copy and pasted this from the polling crate? If so, we can't accept that without a copyright notice. So, we'll likely simply not accept the code.

Yes. I haven't yet added a copyright notice, because I'm not exactly sure how it is supposed to look. Both libraries use the MIT license (polling allows you to choose either Apache or MIT), but neither have a copyright header in any of their files. Do you want a proper copyright notice with a fully blown header or rather a statement, that the code has been taken from polling?

[Thomasdezeeuw]

Yes. I haven't yet added a copyright notice, because I'm not exactly sure how it is supposed to look. Both libraries use the MIT license (polling allows you to choose either Apache or MIT), but neither have a copyright header in any of their files.

That's because all files have the same copyright, so it's not needed to add one to each file.

Do you want a proper copyright notice with a fully blown header or rather a statement, that the code has been taken from polling?

No, I'm afraid it likely means this code will not be accepted.

[Kestrer]

Well, as the sole contributor to polling’s poll.rs implementation I do give you permission to add this code to Mio if you want.

[Thomasdezeeuw]

Well, as the sole contributor to polling’s poll.rs implementation I do give you permission to add this code to Mio if you want.

That solves that then. 👍

[Janrupf]

An update on this PR: I have implemented tracking fd's in userspace and hopefully removed all breaking API changes. The poll selector now uses the same technique as the Win32 implementation for edge based triggering. This in return also means, that it behaves the same way.

I found one case where mio wasn't following its own docs, according to which one needs to ensure WouldBlock is returned before further readiness events can be received (epoll and kqueue don't have that problem, but the Win32 implementation and poll do actually suffer from this limitation).

I think this PR in general is ready for another code review pass - I'm still figuring out a CI solution (turns out mio went from crashing on haiku to crashing haiku, so my next attempt for CI will be Solaris [which also only has poll]).

The force-old-poll feature will be removed once I confirm this to be working on an OS which has only poll.

[Thomasdezeeuw]

An update on this PR: I have implemented tracking fd's in userspace and hopefully removed all breaking API changes. The poll selector now uses the same technique as the Win32 implementation for edge based triggering. This in return also means, that it behaves the same way.

👍

I found one case where mio wasn't following its own docs, according to which one needs to ensure WouldBlock is returned before further readiness events can be received (epoll and kqueue don't have that problem, but the Win32 implementation and poll do actually suffer from this limitation).

To be clear the docs say that:

there is no guarantee that another readiness event will be delivered

Basically, without properly draining the readiness events Mio doesn't guarantee you'll get another event, but the underlying OS might return a readiness event any way. Note however that for Windows we control that behaviour and we're thinking about changing it (see https://github.com/tokio-rs/mio/issues/1611), basically following epoll and kqueue.

I think this PR in general is ready for another code review pass - I'm still figuring out a CI solution (turns out mio went from crashing on haiku to crashing haiku, so my next attempt for CI will be Solaris [which also only has poll]).

Wait... we crash haiku? Any way any OS with just poll (and nothing better) will do. I'll try a do review pass this weekend.

The force-old-poll feature will be removed once I confirm this to be working on an OS which has only poll.

👍

[waddlesplash]

I'm still figuring out a CI solution (turns out mio went from crashing on haiku to crashing haiku

Is this with a nightly build or the beta? If you have a reliable reproducer then please open a ticket with Haiku and we will investigate (and probably fix within days.)

[pheki]

@Janrupf are you still working on this?

[Janrupf]

Not really - the implementation itself was (and still is) working, but I have no way to proof it using a CI pipeline or something along those lines (at least not with the constraints previously set by the maintainers). There are now a bunch of merge conflicts as you can see.

I'm using an implementation of this in a production environment where it has been running on a few hundred embedded devices without any issues, but that's about as good as it gets. If you have interest on picking this up or getting it merged, I'm willing to help.

[notgull]

We test it in polling by using an environment variable to conditionally compile it for Linux, see here.

[Thomasdezeeuw]

We test it in polling by using an environment variable to conditionally compile it for Linux, see here.

The problem with that is that I'm not willing to maintain this implementation on any OS that supports something "better", e.g. epoll on Linux (better might be debatable here, but let's not). Having 1 implementation per OS is enough already.

[pheki]

On the following month I'll try implementing CI for one of the poll-only OSes then.

[pheki]

Hey, I've managed to compile and run the tests using qemu, but it takes a lot longer to run. Even if we optimize and cache in a lot of different ways, like building our own haiku image with rust pre-installed and running cargo vendor before copying the library into the VM, I think it would still take at least 15 minutes to run , while also being a lot more complex. See 1 and 2. The first one was hanging and I cancelled, just like it happened in my local (and fast) qemu, so I suppose it's an implementation issue.

From the issue #1682, it looks like you're willing to reconsider unsing a cfg exclusively to run tests on linux? We could have a mio_unsupported_force_poll cfg which would switch the implementation to poll to be used just to test the poll implementation, like here. This way we could also make this PR a lot simpler, as it would only implement poll, not full Haiku support. If there is interest on merging Haiku support in the future it can be done by someone else.

[Thomasdezeeuw]

@pheki (see first part of your comment below)

From the issue #1682, it looks like you're willing to reconsider unsing a cfg exclusively to run tests on linux? We could have a mio_unsupported_force_poll cfg which would switch the implementation to poll to be used just to test the poll implementation, like here. This way we could also make this PR a lot simpler, as it would only implement poll, not full Haiku support. If there is interest on merging Haiku support in the future it can be done by someone else.

I've documented my thoughts on it in https://github.com/tokio-rs/mio/pull/1684, but yes let's continue with a mio_unspported_force_poll_poll flag, which we can use in the CI. But note that this means poll will not be officially supported for anything but (I think) Haiku.

Hey, I've managed to compile and run the tests using qemu, but it takes a lot longer to run. Even if we optimize and cache in a lot of different ways, like building our own haiku image with rust pre-installed and running cargo vendor before copying the library into the VM, I think it would still take at least 15 minutes to run , while also being a lot more complex. See 1 and 2. The first one was hanging and I cancelled, just like it happened in my local (and fast) qemu, so I suppose it's an implementation issue.

I still like to see some kind of Haiku support in the CI if we want to officially support it, even if it's just a cargo check and then we test the actual implementation on Linux with the cfg flag (not ideal, but it will have to do). We can only run it for merges into the master branch so that the prs aren't blocked on slow CI.

Thank you for looking to this.

[jasta]

I'm interested in throwing my hat in the ring here to try to help get this over the line (and with luck I can get some other esp-rs folks onboard!). Before I get too far though I want to double check that I understand the issues, which AFAICT comes down to two missing requirements:

1. Automated tests covering a specific platform that is intended to (not just can) use the poll() code path introduced in this PR
2. A reliable story around ownership/maintenance for one or more of the platform in (1).

If this understanding is correct, I think the major issue we're facing right now is that the kinds of platforms that want the poll support aren't well set up for efficient, low cost automation which of course makes (1) difficult. Separately, (2) is complicated by the fact that these platforms don't tend to be stable over long periods of time and many dozens of such platforms have come and gone while Linux persists. This doesn't mean they don't deserve support, it just means that the overall community of people is likely going to be a tiny fraction of Linux. Combine that with the fact that the Rust community is a small fraction of developers as a whole and in practice you can never expect to find a large group of committed maintainers. An unfortunate situation for both tokio maintainers and the communities that would benefit from their great work. I think we can find good solutions and move forward though, hopefully others agree :)

Let's take a closer look at (1) first. As an example, esp-idf requires a lot of extra build support and tools (e.g. https://github.com/esp-rs/embuild) as well as a somewhat unsupported qemu fork (https://esp-rs.github.io/book/tooling/simulating/qemu.html) to run the code. I'm confident that we could get this off the ground to run in github's CI, but I doubt very much that we'd ever be able to get the performance good enough to run within a couple of minutes. Realistically I expect this will be 10+ minutes as @pheki has discovered with Haiku as well. Most of the embedded community chooses instead to write platform agnostic code that can be tested on the host, as is the case with this PR being able to run on Linux, then move the rest to run on real hardware that requires significant overhead and cost to maintain in a CI infrastructure. Even companies with deep pockets do it this way just simply because it can become a real money pit to try to get $6 devices to ever be as robust and reliable as Linux rackmount servers (trust me, I tried to do this at Meta for quite some time hehe).

While solutions do exist, I do think we need to maybe reexamine how flexible the requirements really are. For example, we are already in the business of pragmatic support for platforms like Android and iOS despite being unable to test those reliably in GitHub CI as well for much the same reasons. I'd propose that we're relying on the fact that a good number of supported targets are "close enough" to FreeBSD, Linux, Windows, and OS X that testing on those should be sufficient to cover the rest. The real issue with (1) then is that there are no platforms currently that actually would use the poll code path and so the tests for this code feel arbitrary and contrived to use Linux just because it technically supports poll. I agree here (and actually I agree with everything @Thomasdezeeuw has been saying), but consider for a moment that what we're really testing here is that Linux, a POSIX-compliant platform, should work the same as any other POSIX-compliant platform, in the same way we're saying that epoll on any obscure configuration of Linux should work the same as our desktop or rackmount server Linux configurations. Definitely not literally true, but true enough to say we've done our due diligence by relying on reliable, stable standards and doing automated testing against a well known and correct implementation. This actually makes the case stronger for using Linux with poll instead of, say, esp-idf's newlib poll which might be buggy and not a good proxy for, say, Haiku.

For (2), I personally think we can find strong partners in the embedded community, and in particular esp-rs which is a large and growing community with committed maintainers across a number of components. I initially assumed that the gap to get tokio support for esp32 was so large that it wouldn't be practical even technically but honestly @Janrupf 's work will likely inspire others to step up and commit to getting this work landed and maintain it going forward, myself included.

So I guess all that novel was really to say, what exactly do I need to do to get this landed? I see the PR is missing some of the cfg changes you proposed, and a few things could use tidying per your review. The CI checks are currently failing so obviously we gotta fix that. Is there anything else that's insufficient? What more do we need to convince tokio maintainers that we've got the right long term support? I know that for example Rust async has a big push from Espressif as evidenced by Scott Mabin's blog posts: https://mabez.dev/blog/posts/esp-rust-30-06-2023/. Probably not a stretch to imagine I could convince the esp-rs maintainers to commit to having tokio support on the roadmap...

[Janrupf]

Hi @jasta, I just want to add as a note that I'm in fact using this with tokio on the ESP32. I created a support PR on the Tokio repository some time ago, which should contain the changes neccessary to also get tokio to run this. Once mio works as expected it's only a matter of compiling Tokio with the right features.

And thanks a bunch for the long comment, I really appreciate it!

Edit: I'm not home at the moment, but I may have unpushed changes which might make further improvements. I'll check once I'm hone

[Thomasdezeeuw]

I'm interested in throwing my hat in the ring here to try to help get this over the line (and with luck I can get some other esp-rs folks onboard!). Before I get too far though I want to double check that I understand the issues, which AFAICT comes down to two missing requirements:

1. Automated tests covering a specific platform that is intended to (not just can) use the poll() code path introduced in this PR

Ideally the tests don't need to be modified (except for maybe adding a cfg(target_os = "haiku") or similar). Because that means the behaviour for the poll(2) implementation is the same as the on the other platforms. Since Haiku isn't supported by rustup (yet) it's ok to add a mio_unsupported_force_poll_poll flag to force the use of the poll(2) implementation so we can test on Linux on the CI. See https://github.com/tokio-rs/mio/pull/1684 for docs and https://github.com/tokio-rs/mio/pull/1685 for an example.

2. A reliable story around ownership/maintenance for one or more of the platform in (1).

This is pretty important, but mostly for Haiku or other OS(s) that I don't use. Basically if something breaks on Haiku I want to ask somewhere with at least the OS running to help. With the mio_unsupported_force_poll_poll I should be able to reproduce most issues on Linux, but not the one specific to Haiku (or other OS that uses the poll(2) implementation).

If this understanding is correct, I think the major issue we're facing right now is that the kinds of platforms that want the poll support aren't well set up for efficient, low cost automation which of course makes (1) difficult. Separately, (2) is complicated by the fact that these platforms don't tend to be stable over long periods of time and many dozens of such platforms have come and gone while Linux persists. This doesn't mean they don't deserve support, it just means that the overall community of people is likely going to be a tiny fraction of Linux. Combine that with the fact that the Rust community is a small fraction of developers as a whole and in practice you can never expect to find a large group of committed maintainers. An unfortunate situation for both tokio maintainers and the communities that would benefit from their great work. I think we can find good solutions and move forward though, hopefully others agree :)

This is spot on, we all have limited time so for me (other another Mio maintainer) to spend hours trying to setup a Haiku (or other) machine just to fix an issue is quite a tough ask. That is why (2) is important, for people with Haiku already running it can be a 2 minute check to at least reproduce an issue.

Let's take a closer look at (1) first. As an example, esp-idf requires a lot of extra build support and tools (e.g. https://github.com/esp-rs/embuild) as well as a somewhat unsupported qemu fork (https://esp-rs.github.io/book/tooling/simulating/qemu.html) to run the code. I'm confident that we could get this off the ground to run in github's CI, but I doubt very much that we'd ever be able to get the performance good enough to run within a couple of minutes. Realistically I expect this will be 10+ minutes as @pheki has discovered with Haiku as well. Most of the embedded community chooses instead to write platform agnostic code that can be tested on the host, as is the case with this PR being able to run on Linux, then move the rest to run on real hardware that requires significant overhead and cost to maintain in a CI infrastructure. Even companies with deep pockets do it this way just simply because it can become a real money pit to try to get $6 devices to ever be as robust and reliable as Linux rackmount servers (trust me, I tried to do this at Meta for quite some time hehe).

While solutions do exist, I do think we need to maybe reexamine how flexible the requirements really are. For example, we are already in the business of pragmatic support for platforms like Android and iOS despite being unable to test those reliably in GitHub CI as well for much the same reasons. I'd propose that we're relying on the fact that a good number of supported targets are "close enough" to FreeBSD, Linux, Windows, and OS X that testing on those should be sufficient to cover the rest. The real issue with (1) then is that there are no platforms currently that actually would use the poll code path and so the tests for this code feel arbitrary and contrived to use Linux just because it technically supports poll. I agree here (and actually I agree with everything @Thomasdezeeuw has been saying), but consider for a moment that what we're really testing here is that Linux, a POSIX-compliant platform, should work the same as any other POSIX-compliant platform, in the same way we're saying that epoll on any obscure configuration of Linux should work the same as our desktop or rackmount server Linux configurations. Definitely not literally true, but true enough to say we've done our due diligence by relying on reliable, stable standards and doing automated testing against a well known and correct implementation. This actually makes the case stronger for using Linux with poll instead of, say, esp-idf's newlib poll which might be buggy and not a good proxy for, say, Haiku.

Agreed, testing on Linux with mio_unsupported_force_poll_poll would be good enough. Of course this assumes that the Linux poll(2) implementation works exactly like the implementation found in Haikue/esp-idf/etc., which is a big if, but it's (I think) the best we can realistically do right now.

For (2), I personally think we can find strong partners in the embedded community, and in particular esp-rs which is a large and growing community with committed maintainers across a number of components. I initially assumed that the gap to get tokio support for esp32 was so large that it wouldn't be practical even technically but honestly @Janrupf 's work will likely inspire others to step up and commit to getting this work landed and maintain it going forward, myself included.

👍

So I guess all that novel was really to say, what exactly do I need to do to get this landed? I see the PR is missing some of the cfg changes you proposed, and a few things could use tidying per your review. The CI checks are currently failing so obviously we gotta fix that. Is there anything else that's insufficient? What more do we need to convince tokio maintainers that we've got the right long term support? I know that for example Rust async has a big push from Espressif as evidenced by Scott Mabin's blog posts: https://mabez.dev/blog/posts/esp-rust-30-06-2023/. Probably not a stretch to imagine I could convince the esp-rs maintainers to commit to having tokio support on the roadmap...

From my end:

• Add the mio_unsupported_force_poll_poll flag as discussed above.
• Use the mio_unsupported_force_poll_poll to the poll(2) implementation on Linux. Maybe FreeBSD as well so we have at least two different poll(2) implementations we test, so that we don't depend on Linux's specific behaviours/bugs in poll(2)?
• No changes to the tests, ideally at least, so that we know Mio stays cross platform. Exception here for things like add cfg(target_os = "haiku").
• And of course a rebase + passing CI.

I think that about it?

(note that the list might not seem to long, but I only recently agreed to the mio_unsupported_force_poll_poll flag, this was mainly stuck on getting Haiku or poll(2) implementations specific support in the CI)

[jasta]

Ok I'm going to work on the technical aspects of this first:

1. Use mio_unsupported_force_poll_poll
2. Rebase + get CI checks passing generally
3. Add a CI configuration for the tests in GitHub Actions and make sure all that passes

Once I'm ready I'll submit a new PR and then start socializing the maintenance issues a bit with the esp-rs folks. I'm feeling really good that we can get this over the line soon-ish!

[jasta]

I noticed a couple of bugs in the poll implementation while doing this work that result directly from inconsistencies to how the Windows implementation works. For example, when poll yields POLLHUP the edge-triggered IoSourceState doesn't work properly and then the caller just repeatedly gets spammed POLLHUP because the fd is not deregistered internally. The Windows implementation solves this by using a Mutex and shared state between IoSourceState and the Selector, so I think I'll just further copy that behaviour.

This will fix a failing test I'm seeing locally in tcp_stream.rs:tcp_reset_close_event

[jasta]

Only one issue left to fix and I could use your help @Janrupf. Specifically I bumped into: https://github.com/tokio-rs/tokio/issues/5866 which it looks like you should've hit as well unless you were only testing recv_from and send_to APIs or anything else not covered by PollEvented.

[Thomasdezeeuw]

Only one issue left to fix and I could use your help @Janrupf. Specifically I bumped into: tokio-rs/tokio#5866 which it looks like you should've hit as well unless you were only testing recv_from and send_to APIs or anything else not covered by PollEvented.

Also see https://github.com/tokio-rs/mio/issues/1611. We could also decide that reading less then buf.len() bytes should trigger reregistering, matching what epoll and kqueue do already (with edge triggers as used by Mio).

[waddlesplash]

FWIW, a (very basic, FDs/PROC only) implementation of kqueue is being worked on and should make its way into Haiku nightly builds before too long, hopefully removing the need for poll there.

Above, however, it was noted that some versions of this change caused Haiku to crash. Is that reproducible? Do you have a reproducer, or even just a screenshot of the crash?

[Thomasdezeeuw]

FWIW, a (very basic, FDs/PROC only) implementation of kqueue is being worked on and should make its way into Haiku nightly builds before too long, hopefully removing the need for poll there.

Nice. We still need the poll(2) based implementation for ESP-IDF, see https://github.com/tokio-rs/mio/pull/1687 and https://github.com/tokio-rs/mio/pull/1692.

Above, however, it was noted that some versions of this change caused Haiku to crash. Is that reproducible? Do you have a reproducer, or even just a screenshot of the crash?

[psumbera]

FWIW, a (very basic, FDs/PROC only) implementation of kqueue is being worked on and should make its way into Haiku nightly builds before too long, hopefully removing the need for poll there.

Nice. We still need the poll(2) based implementation for ESP-IDF, see #1687 and #1692.

I would also hope that Solaris would be able to use poll(2) too. See #1152.

[Janrupf]

Superseded by #1687, thanks to everyone who continued this!

[Thomasdezeeuw]

Thanks @Janrupf for the implementation!