tokio: Possibly fragile coordination between mio and tokio for Windows event-triggered simulation

Version v1.29.1

Platform Windows

Description I have been working on adding poll support to mio (https://github.com/tokio-rs/mio/pull/1687) and came across a surprising gotcha that I couldn’t figure out where if I did buffered reads as opposed to single byte reads I would get into states where short reads would cause the socket to get stuck and no longer report readiness. I pored over differences between Windows (which also mimics edge-triggered events) and my poll impl only to find nothing until I started debugging from tokio’s side and found this:

https://github.com/tokio-rs/tokio/blob/master/tokio/src/io/poll_evented.rs#L169

This got me thinking if we wanted to clean this up and make tokio a little bit more consistent in how it uses mio we have a few options:

  1. Leave it alone and add cfg!(mio_unsupported_force_poll_poll) to match the windows check. This seems like the easiest option but might end up really hard to maintain because mio_unsupported_force_poll_poll hopefully will eventually be supported for target_os=“espidf” which would mean we would need to make sure to update tokio when that time comes otherwise an innocent mio diff will break the entire feature. In other words, this semi-fragile hack in tokio will become a very fragile hack if poll support is merged.
  2. Modify IoSource to have an extra method that passes a flag whether it was a short read or write. Lots of code churn here, but the tests should cover every case I’d bet and in fact the tests can be modified to prove the code is more correct than it was before (e.g. the tcp_stream test assert can be dropped as per https://github.com/tokio-rs/mio/pull/1687/files#r1259147052)
  3. Modify mio to pass back hints as to whether readiness can be cleared or not. ~This is disruptive for sure, but it has the nice effect of being able to expand the clear_readiness hack to other APIs like UDP’s recv_from. The original motivation was for performance (see https://github.com/tokio-rs/tokio/commit/28ce4eeab2fe983a159b6f6d5070c16e6ff705b9), so why not expand the wins to other cases?~

I personally prefer (2) as it’s probably the least disruptive and fragile, but I’m happy to proceed however ya’ll prefer.

About this issue

  • Original URL
  • State: open
  • Created a year ago
  • Comments: 16 (16 by maintainers)

Commits related to this issue

Most upvoted comments

Technically, Tokio is depending on platform specific behaviour here (https://github.com/tokio-rs/tokio/blob/master/tokio/src/io/poll_evented.rs#L169). It works only because we currently only support epoll and kqueue on Unix, but as @jasta mentioned with poll that’s about to change. That said, I personally also depend on this behaviour.

We have an open issue to change this: https://github.com/tokio-rs/mio/issues/1611. But there is one catch which is try_io which doesn’t get access to any buffers and or read/written number of bytes, so determining a short read/write/etc. is difficult.

The most correct way to resolve this is to remove the short read/write optimisation. But I understand no one wants the loss of performance. So checking for mio_unsuported_force_poll_poll would work… for a while at least, until we add targets that only support the poll(2) implementation (e.g. espidf or haiku), then it will break again (in a hard to debug way). We could go ahead with https://github.com/tokio-rs/mio/issues/1611, but I think we should consider that a breaking change, but I’m not sure yet.

I would check for mio_unsuported_force_poll_poll for now as a short term solution and we can communicate Mio <-> Tokio when we start adding poll(2) targets without the use of mio_unsuported_force_poll_poll.