Handle ERROR_MORE_DATA for named pipes #1921
Merged
+131
−6
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This MR addresses an issue where a panic occurs while attempting to read messages larger than the buffer size for Windows named pipes (this is hardcoded to 4096 bytes).
Root Cause
Per MS docs:
Furthermore, if the provided buffer to the overlapped read operation has free capacity, data would be copied into it. However, since no special handling is given to
ERROR_MORE_DATA, thebytes transfered == 0assertion would be executed, causing the above panic (where 4096 is the number of bytes read despite anERROR_MORE_DATAerror).Solution
The core idea is to gracefully handle
ERROR_MORE_DATA, and subsequently transition the named pipe's internal state back to aState::Ok(buf, 0), wherebufis any unread data; then repeat this until the last set of bytes of the message is copied, andERROR_MORE_DATAis no longer returned.For added context (given this part of the code is poorly documented): the repeat until no
ERROR_MORE_DATAis achieved when an overlapped read is scheduled viaschedule_read(..), and on an I/O completion port event,read_done(...)being called to transition to an "end" state in preparation for the nextRead::read(..)call.I've also added relevant tests to verify the new behaviour.
Related Issues
panic assert failed left: 4096 right: 0- when sending > 4096 bytes in windows pipe message mode tokio#6460Other Observations
I noticed tokio-rs/tokio#5307 is also used to track this issue. However, I believe they're two distinct problems. This handles the case where more data needs to be read, but can't because we panic mid-way, while the other is more concerned about tweaking the buffer size of the named pipe (which imo is up to the implementer).
BTW, one could achieve similar fix by leveraging a larger buffer (tokio-rs/tokio#5307) since there'd be enough capacity for the entire message to fit in. But that has its own tradeoffs primarily due to the large buffer size.