Skip to content

Disable docker system dial-stdio on Windows - #1710

Merged
silvin-lubecki merged 1 commit into
docker:masterfrom
ijc:no-dial-stdio-on-windows
Mar 11, 2019
Merged

Disable docker system dial-stdio on Windows#1710
silvin-lubecki merged 1 commit into
docker:masterfrom
ijc:no-dial-stdio-on-windows

Conversation

@ijc

@ijcijc commented Mar 4, 2019

Copy link
Copy Markdown
Contributor

The conn here is *winio.win32MessageBytePipe which does not have a
CloseRead method (it does have CloseWrite) resulting in:

docker@WIN-NUC0 C:\Users\docker>.\docker-windows-amd64.exe system dial-stdio
the raw stream connection does not implement halfCloser

Also disable the path which uses this for cli-plugins on Windows.

Signed-off-by: Ian Campbell ijc@docker.com

/cc @tonistiigi@AkihiroSuda if Windows can't be fixes soon perhaps we should do this and revert when Windows works?

The `conn` here is `*winio.win32MessageBytePipe` which does not have a
`CloseRead` method (it does have `CloseWrite`) resulting in:
docker@WIN-NUC0 C:\Users\docker>.\docker-windows-amd64.exe system dial-stdio
the raw stream connection does not implement halfCloser
Also disable the path which uses this for cli-plugins on Windows.
Signed-off-by: Ian Campbell <ijc@docker.com>
@AkihiroSuda

Copy link
Copy Markdown
Collaborator

According to #1376 it works on Windows with certain configuration?

@AkihiroSuda

Copy link
Copy Markdown
Collaborator

I think CloseRead can be added as NOP

@ijc

ijc commented Mar 5, 2019

Copy link
Copy Markdown
ContributorAuthor

According to #1376 it works on Windows with certain configuration?

Ah, I guess there it is a proper tcp socket (which I would guess does indeed support half close in both directions since it is tcp) since it is doing ssh://.

For me (using dial-stdio in a plugin to connect back to the local dameon) it is (I presume) the named pipe, which I suppose doesn't.

@ddebroy

Copy link
Copy Markdown

/cc @andrey-ko for taking a look at the Windows side of this.

@ijc

ijc commented Mar 6, 2019

Copy link
Copy Markdown
ContributorAuthor

@thaJeztah I think this is only tangentially related to plugins (cf the label)

@ijc

ijc commented Mar 6, 2019

Copy link
Copy Markdown
ContributorAuthor

#1718 is my attempt to fix this on Windows, PTAL (my testing on Windows has been rather limited)

ulyssessouza pushed a commit to docker-archive-public/docker.app that referenced this pull request Mar 7, 2019
- Update docker/cli is now pointing to ulyssessouza/cli.
The change needs the merge of docker/cli#1710
and docker/cli#1690
- Fix issues relative paths in Jenkinsfile and Jenkinsfile.baguette
- Avoid using '--config' in favor of env variable 'DOCKER_CONFIG'
Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>
chris-crone pushed a commit to chris-crone/app that referenced this pull request Mar 7, 2019
- Update docker/cli is now pointing to ulyssessouza/cli.
The change needs the merge of docker/cli#1710
and docker/cli#1690
- Fix issues relative paths in Jenkinsfile and Jenkinsfile.baguette
- Avoid using '--config' in favor of env variable 'DOCKER_CONFIG'
Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>

@thaJeztahthaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as a temporary solution while we work on #1718

@silvin-lubeckisilvin-lubecki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@silvin-lubecki
silvin-lubecki merged commit b86bff8 into docker:masterMar 11, 2019
@GordonTheTurtleGordonTheTurtle added this to the 19.03.0 milestone Mar 11, 2019
@silvin-lubecki

Copy link
Copy Markdown
Contributor

Merged, to be reverted when #1718 will land.

@ijc
ijc deleted the no-dial-stdio-on-windows branch March 11, 2019 14:32
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@ijc@AkihiroSuda@ddebroy@silvin-lubecki@thaJeztah@GordonTheTurtle