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

Move closeNotify to fix panic with newer golang #19420

Merged
merged 1 commit into from Jan 19, 2016

Conversation

Projects
None yet
7 participants
@clnperez
Copy link
Contributor

commented Jan 18, 2016

This is happening now due to improvements in net/http:
golang/go@99fb191

To test, change the go version in the Dockerfile:
-ENV GO_VERSION 1.5.3
+ENV GO_VERSION 1.6beta2

More info here: golang/go#14001

Signed-off-by: Christy Perez christy@linux.vnet.ibm.com

Move closeNotify to fix panic with newer golang
This is happening now due to improvements in net/http:
golang/go@99fb191

To test, change the go version in the Dockerfile:
-ENV GO_VERSION 1.5.3
+ENV GO_VERSION 1.6beta2

More info here: golang/go#14001

Signed-off-by: Christy Perez <christy@linux.vnet.ibm.com>
@runcom

This comment has been minimized.

Copy link
Member

commented Jan 18, 2016

LGTM

As I see in the linked golang issue Brad is saying code was probably racy before this fix so maybe it's worth considering adding this to the 1.10 milestone @thaJeztah wdyt

@tiborvass tiborvass added this to the 1.10.0 milestone Jan 18, 2016

@tiborvass

This comment has been minimized.

Copy link
Collaborator

commented Jan 18, 2016

LGTM

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Jan 18, 2016

agreed, thanks!

clnperez added a commit to clnperez/moby that referenced this pull request Jan 18, 2016

Move CloseNotify to fix race condition with go1.6
Equivalent to upstream moby#19420

Signed-off-by: Christy Perez <christy@linux.vnet.ibm.com>
@tiborvass

This comment has been minimized.

Copy link
Collaborator

commented Jan 18, 2016

@clnperez

21:16:49 
21:16:49 ----------------------------------------------------------------------
21:16:49 FAIL: check_test.go:130: DockerTrustSuite.SetUpTest
21:16:49 
21:16:49 check_test.go:132:
21:16:49     s.not = setupNotary(c)
21:16:49 github.com/docker/docker/integration-cli/_test/_obj_test/trust_server.go:136:
21:16:49     ...open github.com/docker/docker/integration-cli/_test/_obj_test/trust_server.go: no such file or directory
21:16:49 ... Error: Timeout waiting for test notary to become available: Get https://localhost:4443/v2/: dial tcp [::1]:4443: getsockopt: connection refused
21:16:49 
21:16:50 
21:16:50 ----------------------------------------------------------------------
21:16:50 PANIC: docker_cli_build_test.go:5831: DockerTrustSuite.TestBuildContextDirIsSymlink
21:16:50 
21:16:50 ... Panic: Fixture has panicked (see related PANIC)
21:16:50 
21:16:50 ----------------------------------------------------------------------
@runcom

This comment has been minimized.

Copy link
Member

commented Jan 18, 2016

Is it related?!

@tophj-ibm

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2016

#19393
interesting I thought this was just a ppc64le/arm issue but now it's on janky as well.

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Jan 18, 2016

I'll kick Janky

@cpuguy83

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2016

Yeah, not related I've seen this in another PR as well.
Tests all green now. Merging.

cpuguy83 added a commit that referenced this pull request Jan 19, 2016

Merge pull request #19420 from clnperez/close-notify-fix
Move closeNotify to fix panic with newer golang

@cpuguy83 cpuguy83 merged commit b6be645 into moby:master Jan 19, 2016

7 checks passed

docker/dco-signed All commits signed
Details
documentation success 1 tests run, 1 skipped, 0 failed.
Details
experimental Jenkins build Docker-PRs-experimental 13799 has succeeded
Details
gccgo Jenkins build Docker-PRs-gccgo 678 has succeeded
Details
janky Jenkins build Docker-PRs 22596 has succeeded
Details
userns Jenkins build Docker-PRs-userns 5047 has succeeded
Details
windows Jenkins build Windows-PRs 20500 has succeeded
Details
@clnperez

This comment has been minimized.

Copy link
Contributor Author

commented Jan 19, 2016

Nice. Thanks everyone.

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Jan 19, 2016

Thank you for contributing, @clnperez !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.