Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conditional reregistering interests in IoSourceState with mio_unsupported_force_poll_poll poll(2) based on IO operation result #1809

Closed
flxo opened this issue Jun 19, 2024 · 5 comments

Comments

@flxo
Copy link

flxo commented Jun 19, 2024

Hello,

I'm playing mio (as dependency of Tokio) on Fuchsia. Since Fuchsia lacks epoll support, I opted to use mio_unsupported_force_poll_poll. Sidenote: The waker implementation via eventfd(2) appears to be functioning correctly on Fuchsia. Building mio for Fuchsia required some minor adjustments, primarily extending cfg gates in various places, such as tcp.rs.

My test setup includes a TCP (IPv6) listener that spawns a task for each connection, echoing data via tokio::io::copy. The client is either socat or a TcpStream in another task. I observed that the initial accept and the first data transaction work fine, but subsequent communication stalls.

The issue appears to be that the interests in the poll implementation are cleared whenever a poll returns revents, as seen here.

I compared the behavior on an x86 Linux system using the same test and analyzed the logs. The difference is that on Linux, the interest of a file descriptor (fd) is cleared and re-registered if the I/O operation returns WouldBlock as shown here.

On Fuchsia, however, the I/O operation returns (in most cases) Ok(..), and the interest is never re-registered, leading to the communication stall. Could this happen on Linux as well?

What's the rationale behind reregistering only on WouldBlock? My assumption is that e.g a read could return Ok(..) and the fd should be reregistered afterwards. I didn't dig into the Linux side yet.

Testwise I removed if let Err(..) condition around the reregistering and successfully smoke tested some example Tokio code and the mio tcp_server example .

I can share the modifications I did to mio. Same for updates on the integration of mio in Fuchsia that enable net and os-poll (+ Tokio features) if needed.

I know that mio_unsupported_force_poll_poll. I know that Fuchsia is unspported. :-)

Any thoughts on this?

Thanks! @flxo

@Thomasdezeeuw
Copy link
Collaborator

I'm playing mio (as dependency of Tokio) on Fuchsia. Since Fuchsia lacks epoll support, I opted to use mio_unsupported_force_poll_poll. Sidenote: The waker implementation via eventfd(2) appears to be functioning correctly on Fuchsia. Building mio for Fuchsia required some minor adjustments, primarily extending cfg gates in various places, such as tcp.rs.

I ported Mio to Fuchsia in #1811. Hope that makes things a bit easier.

What's the rationale behind reregistering only on WouldBlock? My assumption is that e.g a read could return Ok(..) and the fd should be reregistered afterwards. I didn't dig into the Linux side yet.

Per Mio's documentation in https://docs.rs/mio/latest/mio/struct.Poll.html#readiness-operations. Mio's I/O types need to be "drained", that means calling the I/O method (e.g. read) in a loop until it returns WouldBlock. This is true for all implementations. However, for some implementations the I/O interest is already reregistered within the kernel when e.g. a read a shorter then the buffer. This is however platform specific behaviour that we can't make work across platforms. Hence the requirement for the draining (again the see the linked docs above).

Tokio has done this wrong in the past where it assumed that all Unix implementations had the in-kernel reregister on short read behaviour. In this if you search for espidf, which also uses the poll(2) implementation, you'll find the relevant code. But this is an issue you need to raise with Tokio as technically they are not adhering to Mio's API usage requirements.

@flxo
Copy link
Author

flxo commented Jun 25, 2024

I ported Mio to Fuchsia in #1811. Hope that makes things a bit easier.

Thanks for that. I'll prepare a PR to Fuchsia once mio is published with Fuchsia support. I tested basically the same.

What's the rationale behind reregistering only on WouldBlock? My assumption is that e.g a read could return Ok(..) and the fd should be reregistered afterwards. I didn't dig into the Linux side yet.

Per Mio's documentation in https://docs.rs/mio/latest/mio/struct.Poll.html#readiness-operations. Mio's I/O types need to be "drained", that means calling the I/O method (e.g. read) in a loop until it returns WouldBlock. This is true for all implementations. However, for some implementations the I/O interest is already reregistered within the kernel when e.g. a read a shorter then the buffer. This is however platform specific behaviour that we can't make work across platforms. Hence the requirement for the draining (again the see the linked docs above).

Tokio has done this wrong in the past where it assumed that all Unix implementations had the in-kernel reregister on short read behaviour. In this if you search for espidf, which also uses the poll(2) implementation, you'll find the relevant code. But this is an issue you need to raise with Tokio as technically they are not adhering to Mio's API usage requirements.

What do you mean with "in the past"? Should this be "fixed"? My testes are done with Tokio 1.32.1 which is the currently integrated version in Fuchsia. Should be easy to update test wise.

I checked espidf specific code in Tokio and mio and couldn't find anything "reregister" related. Can you please give me a hint?

So - I understand that my workaround just hides the missing reregistration from Tokio.

@Thomasdezeeuw
Copy link
Collaborator

What do you mean with "in the past"? Should this be "fixed"? My testes are done with Tokio 1.32.1 which is the currently integrated version in Fuchsia. Should be easy to update test wise.

See tokio-rs/tokio#5866 and tokio-rs/tokio#5881.

I checked espidf specific code in Tokio and mio and couldn't find anything "reregister" related. Can you please give me a hint?

I'm not sure if it's still in but the code added in tokio-rs/tokio#5881 would be it.

So - I understand that my workaround just hides the missing reregistration from Tokio.

Pretty much. Tokio incorrectly (for the poll(2) implementation) clear readiness on short reads.

@flxo
Copy link
Author

flxo commented Jun 25, 2024

conclusion for me is that I need to set mio_unsupported_force_poll_poll when building Tokio as well. Didn't know that. This needs to be done in the fuchsia build dedicated for Tokio. I just set it for mio.
Nothing left to be done within mio (main with #1811).

Thanks @Thomasdezeeuw

@flxo flxo closed this as completed Jun 25, 2024
@Thomasdezeeuw
Copy link
Collaborator

conclusion for me is that I need to set mio_unsupported_force_poll_poll when building Tokio as well.

It shouldn't be needed, but that is being fixes in tokio-rs/tokio#5866, see tokio-rs/tokio#5866 (comment) and comments after.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants