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:
- 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. - 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)
- 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
- poll: Do not clear readiness on short read/writes. The new mio_unsupported_force_poll_poll behaviour works the same as Windows (using level-triggered APIs to mimic edge-triggered ones) and it depends... — committed to jasta/tokio by jasta a year ago
- poll: Do not clear readiness on short read/writes. The new mio_unsupported_force_poll_poll behaviour works the same as Windows (using level-triggered APIs to mimic edge-triggered ones) and it depends... — committed to jasta/tokio by jasta a year ago
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_iowhich 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_pollwould work… for a while at least, until we add targets that only support thepoll(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_pollfor now as a short term solution and we can communicate Mio <-> Tokio when we start addingpoll(2)targets without the use ofmio_unsuported_force_poll_poll.Cc @Noah-Kennedy due to #4840