fix: retry close_notify in poll_shutdown when the send buffer is full - #78
fix: retry close_notify in poll_shutdown when the send buffer is full#78ArniDagur wants to merge 2 commits into
close_notify in poll_shutdown when the send buffer is full#78Conversation
ee2dae6 to
e862e48
Compare
|
I know you guys are busy with the main I would volunteer to help with the maintenance of |
I think we should wait it out a bit to see if the situation with review bandwidth improves as we get on top of rustls 0.24. |
8b3e536 to
f89f17f
Compare
|
Code review comments have been addressed. I squashed the changes into one commit. |
djc
left a comment
djc
left a comment
There was a problem hiding this comment.
Thanks.
It would be nicer to have a separate commit that renames async_read_ready.rs to async_ready.rs, to help engage Git's rename tracking.
The module is about to gain an `AsyncWriteReady` trait next to `AsyncReadReady`, so give it a name that covers both. Renaming it on its own keeps the change visible to Git's rename detection.
`poll_shutdown` marked the write side closed _before_ enforcing a successful `close_notify`. With a full send buffer this resulted in two bugs: 1. Shutdown failed with `WouldBlock`, which is an error type that should not escape a poll-based API. 2. Subsequent retries of the shutdown skipped the `close_notify` entirely, since `write_closed` was already set. We fix this by only marking the write side closed once the alert is sent (or has failed fatally), and retrying on `WouldBlock`. The retry uses a new `AsyncWriteReady` trait, which mirrors the preexisting `AsyncReadReady`. It exposes tokio's `poll_write_ready` and `try_io`. The latter `try_write_io` clears write-readiness when `send_close_notify` returns `WouldBlock`, so the task parks until the socket becomes writable instead of [busy-polling][1]. [1]: https://github.com/rustls/ktls/blob/5e3c7d6ceadbb1ae98d06908d559490723899aed/ktls/src/ktls_stream.rs#L268-L277 This PR introduces a minor breaking change, since the `AsyncWrite` impl for `KtlsStream<IO>` now requires `IO: AsyncWriteReady`. This change also lays the foundation for additional work, including in relation to properly implementing `KeyUpdate`.
f89f17f to
198807b
Compare
This is done |
2 participants
poll_shutdownmarked the write side closed before enforcing a successfulclose_notify. With a full send buffer this resulted in two bugs:WouldBlock, which is an error type that should not escape a poll-based API.close_notifyentirely, sincewrite_closedwas already set.We fix this by only marking the write side closed once the alert is sent (or has failed fatally), and retrying on
WouldBlock.The retry uses a new
AsyncWriteReadytrait, which mirrors the preexistingAsyncReadReady. It exposes tokio'spoll_write_readyandtry_io. The latter clears write-readiness whensend_close_notifyreturnsWouldBlock, so the task parks until the socket becomes writable instead of busy-polling.This PR introduces a minor breaking change, since the
AsyncWriteimpl forKtlsStream<IO>now requiresIO: AsyncWriteReady. This change also lays the foundation for additional work, including in relation to properly implementingKeyUpdate(which I plan on doing).