Skip to content

/cli/command/container/hijack.go: fix tcp half-closed connection unreliability in WSL - #4548

Open
VertexToEdge wants to merge 1 commit into
docker:masterfrom
VertexToEdge:3586-no-output-hello-world-in-wsl2
Open

/cli/command/container/hijack.go: fix tcp half-closed connection unreliability in WSL#4548
VertexToEdge wants to merge 1 commit into
docker:masterfrom
VertexToEdge:3586-no-output-hello-world-in-wsl2

Conversation

@VertexToEdge

@VertexToEdgeVertexToEdge commented Aug 31, 2023

Copy link
Copy Markdown

closes#3586

- What I did
I fixed tcp half-closed connection unreliability.
the problem occurs because of half-closeing hijacked socket. it makes tcp connection closed.

I experimented various of situation(windows<->ubuntu, windows<->hyper-v ubuntu, windows <-> hyper-v windows). but only windows -> WSL(linux) cannot make half-closed connection that is host to guest direction.

- How I did it
I moved h.resp.CloseWrite() to func (h *hijackedIOStreamer) stream(ctx context.Context) error from goroutine in
func (h *hijackedIOStreamer) beginInputStream(restoreInput func()) (doneC <-chan struct{}, detachedC <-chan error)
It secures hijacked socket will send and receive all of data. so it prevents half-closed connection in attach sequence.

image

- How to verify it
before:
image




after:
image

- Description for the changelog
Fixed no output of docker run command over WSL 2 Docker context from Windows via tcp

- A picture of a cute animal (not mandatory but encouraged)

@VertexToEdgeVertexToEdge changed the title fix race condition in /cli/commnad/container/hijack.go/cli/commnad/container/hijack.go: fix race condition in closeing hijacked pipeAug 31, 2023
@VertexToEdgeVertexToEdge changed the title /cli/commnad/container/hijack.go: fix race condition in closeing hijacked pipe/cli/commnad/container/hijack.go: fix race condition in closing hijacked pipeAug 31, 2023
@codecov-commenter

codecov-commenter commented Aug 31, 2023

Copy link
Copy Markdown

Codecov Report

Merging #4548 (2e01d90) into master (5777c1b) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

Additional details and impacted files
@@ Coverage Diff @@## master #4548 +/- ##
==========================================
- Coverage 59.68% 59.65% -0.04% 
==========================================
Files 287 285 -2 Lines 24752 24751 -1 ==========================================
- Hits 14774 14765 -9 - Misses 9092 9098 +6 - Partials 886 888 +2 

@thaJeztahthaJeztah added this to the 25.0.0 milestone Sep 21, 2023
@VertexToEdgeVertexToEdge changed the title /cli/commnad/container/hijack.go: fix race condition in closing hijacked pipe/cli/commnad/container/hijack.go: fix tcp half-closed connection unreliabilitySep 21, 2023
@VertexToEdgeVertexToEdge changed the title /cli/commnad/container/hijack.go: fix tcp half-closed connection unreliability/cli/commnad/container/hijack.go: fix tcp half-closed connection unreliability in WSLSep 21, 2023
@VertexToEdge

Copy link
Copy Markdown
Author

I figured out the real reason of bug. It is not race-condition.
I updated it on this PR description.
Thank you.

@thaJeztah

Copy link
Copy Markdown
Member

Thanks! I asked a colleague who's more familiar with some of this to have a look (but they're on leave currently, so hope they get round to it).

I did notice there's a typo in the commit message (commnad instead of command) if you need to do another update of the PR, could you fix that? 😅

@VertexToEdge

Copy link
Copy Markdown
Author

Oh sorry... Thanks for noticing. I will fix it in 15 hours.

@VertexToEdgeVertexToEdge changed the title /cli/commnad/container/hijack.go: fix tcp half-closed connection unreliability in WSL/cli/command/container/hijack.go: fix tcp half-closed connection unreliability in WSLSep 22, 2023
@VertexToEdge
VertexToEdgeforce-pushed the 3586-no-output-hello-world-in-wsl2 branch from badf9be to 6af01b1CompareSeptember 22, 2023 04:02
@thaJeztah

Copy link
Copy Markdown
Member

@laurazard are you able to help review this one? 🤗

Signed-off-by: Seongbin Hong <vertex@g.cnu.ac.kr>
@thaJeztah
thaJeztahforce-pushed the 3586-no-output-hello-world-in-wsl2 branch from 6af01b1 to 2e01d90CompareDecember 20, 2023 22:39
@thaJeztah

Copy link
Copy Markdown
Member

Looks like this one dropped between the cracks. I did a quick rebase to get a run of CI, and will try to get reviewers

@dmcgowan

Copy link
Copy Markdown
Contributor

I fixed tcp half-closed connection unreliability. the problem occurs because of half-closeing hijacked socket. it makes tcp connection closed.

Isn't the problem then that CloseWrite is closing the stream, not when CloseWrite is called. Write should always be closed after all writes are completed so that the other end of the stream can get the EOF signal and complete its write. Calling CloseWrite after done reading could cause deadlock situations where both ends of the stream are waiting on each other. A basic example would be cat stdio to stdout. It seems that if there is an issue, its in the implementation of CloseWrite rather than it being called incorrectly.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No output when running hello-world without -i over WSL 2 Docker context

5 participants

@VertexToEdge@codecov-commenter@thaJeztah@dmcgowan@vvoland