Request an HTTP upgrade when attaching to stdin - #610
Open
tas50 wants to merge 1 commit into
Open
Conversation
Container#attach(stdin:) hijacks the socket, but never asked the daemon to
upgrade the connection. Docker then answers 200 and leaves the payload on
Excon's buffered socket, which the raw hijacked read never looks at, so the
attach blocks forever with no timeout and no way to interrupt it:
hijack: copying stdin => socket
hijack: closing write end of hijacked socket
hijack: reading from hijacked socket
<hangs indefinitely>
A raw socket performing the same exchange with the upgrade headers works
correctly, so this is the request, not the platform. Sending them makes Docker
reply 101 and hand the socket over, which also means 101 has to be an accepted
status - the default expects list is 200..204 plus 301 and 304, so the reply
was otherwise rejected.
The spec that used to hang now passes, and container_spec can be run to
completion on Docker Desktop for the first time.
Only the stdin branch is touched; attach without stdin is unchanged.
Docker::Exec#start! hijacks the same way and has the same defect. It is left
for a follow-up so this stays reviewable.
This file contains hidden or 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
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.
Container#attach(stdin:)hijacks the socket, but never asked the daemon to upgrade the connection. Docker then answers200and leaves the payload on Excon's buffered socket — which the raw hijacked read never looks at. The attach blocks forever, with no timeout and no way to interrupt it:Why this is the gem, not the platform
A raw socket performing the identical exchange with the upgrade headers works perfectly:
The fix
Send
Connection: Upgrade/Upgrade: tcpon the hijacked request. Docker then replies101 Switching Protocolsand hands the socket over — which also means101has to be an accepted status, since the default expects list is200..204plus301/304and was otherwise rejecting the reply:Verification
The spec that used to hang indefinitely now passes:
And
container_speccan now be run to completion on Docker Desktop for the first time — 60 examples, 2 failures, both confirmed pre-existing on master and both macOS-only (they pass on the Linux runners):#exec when stdin object is passed—Docker::Exec#start!hijacks the same way and has the same defect#export yields each chunk— unrelatedTwo specs assert the contract; both were confirmed failing before the change.
Scope
Only the stdin branch is touched — attach without stdin is unchanged, and its existing specs still pass.
Docker::Exec#start!has the identical defect and is left for a follow-up so this stays reviewable.CI will be red until #605 lands.