Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

VertexToEdge
Copy link

@VertexToEdge VertexToEdge commented Aug 31, 2023

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)

@VertexToEdge VertexToEdge changed the title fix race condition in /cli/commnad/container/hijack.go /cli/commnad/container/hijack.go: fix race condition in closeing hijacked pipe Aug 31, 2023
@VertexToEdge VertexToEdge 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 pipe Aug 31, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 31, 2023

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     

@VertexToEdge VertexToEdge reopened this Sep 17, 2023
@thaJeztah thaJeztah added this to the 25.0.0 milestone Sep 21, 2023
@VertexToEdge VertexToEdge 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 unreliability Sep 21, 2023
@VertexToEdge VertexToEdge 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 WSL Sep 21, 2023
@VertexToEdge
Copy link
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
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
Author

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

@VertexToEdge VertexToEdge 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 WSL Sep 22, 2023
@VertexToEdge VertexToEdge force-pushed the 3586-no-output-hello-world-in-wsl2 branch from badf9be to 6af01b1 Compare September 22, 2023 04:02
@thaJeztah
Copy link
Member

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

Signed-off-by: Seongbin Hong <vertex@g.cnu.ac.kr>
@thaJeztah thaJeztah force-pushed the 3586-no-output-hello-world-in-wsl2 branch from 6af01b1 to 2e01d90 Compare December 20, 2023 22:39
@thaJeztah
Copy link
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
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.

@thaJeztah thaJeztah modified the milestones: 25.0.0, 26.0.0 Jan 19, 2024
@vvoland vvoland modified the milestones: 26.0.0, 27.0.0 Mar 14, 2024
@vvoland vvoland modified the milestones: 27.0.0, v-future Jun 20, 2024
Sign up for free to 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