Flush outgoing content before sending EOF message #43
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This exhibit a deadlock we see sometimes when using docker cli connected to a daemon through a named pipe with some data in the input stream (ie: `echo "hello" | docker run ... `) The problem seems to be that the EOF 0-length message is collapsed with the previous message, if the buffer associated with the named pipe is not previously flushed Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
This forces any pending data to be sent to the remote party before emitting the 0-length EOF message, to make sure it appears as a seperate message. This fixes the "TestEchoWithMessaging" test Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>
|
ping @jstarks PTAL 👍 |
|
@jstarks @thaJeztah This is really important for Docker for Windows. Please TAL |
|
/cc @jhowardmsft |
dgageot
added a commit
to dgageot/docker
that referenced
this pull request
Mar 21, 2017
Fixes moby#31922 Vendor a fork of Microsoft/go-winio that has this PR (microsoft/go-winio#43) merged Signed-off-by: David Gageot <david@gageot.net>
vdemeester
pushed a commit
to vdemeester/moby
that referenced
this pull request
Mar 21, 2017
Fixes moby#31922 Vendor a fork of Microsoft/go-winio that has this PR (microsoft/go-winio#43) merged Signed-off-by: David Gageot <david@gageot.net>
|
@jstarks @jhowardmsft @darrenstahlmsft we need your review on this 👼 Right now we use a patch version of this to fix some issues with docker4windows but it would be way better if it was fixed upstream 👼 🙏 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #42
This adds a
Flush()method onwin32File, and calls it onCloseWrite()just before sending the 0-length EOF message.This makes sure that the 0-length message is not collapsed into the previous message, and that functions like io.Copy() do correctly receive an io.EOF error and completes instead of hanging.
The first commit introduces a test that hangs, the second fixes the issue (and makes the test pass)
This was identified, qualified and fixed by @dgageot and me