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

Jump to bottom

net/http: data race in test #8483

Closed
dvyukov opened this issue Aug 6, 2014 · 7 comments
Closed

net/http: data race in test #8483

dvyukov opened this issue Aug 6, 2014 · 7 comments
Labels
FrozenDueToAge
Milestone
Go1.4

Comments

@dvyukov
Copy link
Member

dvyukov commented Aug 6, 2014 

Freebsd-race builder says:

http://build.golang.org/log/08f6845ff56ba26e380d39f678275ed03072ba19

==================
WARNING: DATA RACE
Read by goroutine 61:
  sync.raceRead()
      /usr/local/go/src/pkg/sync/race.go:37 +0x35
  sync.(*WaitGroup).Add()
      /usr/local/go/src/pkg/sync/waitgroup.go:60 +0xbe
  net/http_test.func·189()
      /usr/home/gopher/racer/work/freebsd-amd64-race-98c597959808/go/src/pkg/net/http/transport_test.go:1074
+0xa6
  net/http.(*Transport).dial()
      /usr/home/gopher/racer/work/freebsd-amd64-race-98c597959808/go/src/pkg/net/http/transport.go:442
+0xcb
  net/http.(*Transport).dialConn()
      /usr/home/gopher/racer/work/freebsd-amd64-race-98c597959808/go/src/pkg/net/http/transport.go:496
+0xab
  net/http.func·022()
      /usr/home/gopher/racer/work/freebsd-amd64-race-98c597959808/go/src/pkg/net/http/transport.go:472
+0x8c

Previous write by goroutine 50:
  sync.raceWrite()
      /usr/local/go/src/pkg/sync/race.go:41 +0x35
  sync.(*WaitGroup).Wait()
      /usr/local/go/src/pkg/sync/waitgroup.go:122 +0x176
  net/http_test.TestTransportConcurrency()
      /usr/home/gopher/racer/work/freebsd-amd64-race-98c597959808/go/src/pkg/net/http/transport_test.go:1110
+0x6a3
  testing.tRunner()
      /usr/local/go/src/pkg/testing/testing.go:427 +0x10b

Goroutine 61 (running) created at:
  net/http.(*Transport).getConn()
      /usr/home/gopher/racer/work/freebsd-amd64-race-98c597959808/go/src/pkg/net/http/transport.go:474
+0x334
  net/http.(*Transport).RoundTrip()
      /usr/home/gopher/racer/work/freebsd-amd64-race-98c597959808/go/src/pkg/net/http/transport.go:201
+0x57e
  net/http.send()
      /usr/home/gopher/racer/work/freebsd-amd64-race-98c597959808/go/src/pkg/net/http/client.go:195
+0x626
  net/http.(*Client).send()
      /usr/home/gopher/racer/work/freebsd-amd64-race-98c597959808/go/src/pkg/net/http/client.go:118
+0x1ff
  net/http.(*Client).doFollowingRedirects()
      /usr/home/gopher/racer/work/freebsd-amd64-race-98c597959808/go/src/pkg/net/http/client.go:343
+0xd27
  net/http.(*Client).Get()
      /usr/home/gopher/racer/work/freebsd-amd64-race-98c597959808/go/src/pkg/net/http/client.go:275
+0xcd
  net/http_test.func·190()
      /usr/home/gopher/racer/work/freebsd-amd64-race-98c597959808/go/src/pkg/net/http/transport_test.go:1087
+0x188

Goroutine 50 (finished) created at:
  testing.RunTests()
      /usr/local/go/src/pkg/testing/testing.go:509 +0xb3e
  testing.Main()
      /usr/local/go/src/pkg/testing/testing.go:440 +0xa2
  main.main()
      net/http/_test/_testmain.go:479 +0xdc
==================

The test expects that Dial function will be called synchronously inside of client.Get.
But it is not. So the test does not wait for some of the dialers.

It could have been the root cause of:
https://golang.org/issue/6970
and some other spurious failures in net/http tests.
@bradfitz
Copy link
Contributor

Comment 1:

CL 126610043

Owner changed to @bradfitz.

Status changed to Started.

@gopherbot
Copy link

Comment 2:

CL https://golang.org/cl/126610043 mentions this issue.

@bradfitz
Copy link
Contributor

Comment 3:

This issue was closed by revision 32c0dce.

Status changed to Fixed.

@dvyukov
Copy link
Member Author

dvyukov commented Sep 3, 2014

Comment 4:

Brad, I think that the race detector actually pointed to a more serious not-test-only
issue.
So if a user creates a http.Client, issues a bunch of requests and then wants to
shutdown it and all opened connections; what is she intended to do? The report suggests
that just waiting for all pending requests and calling CloseIdleConnections won't do, as
there can be new racing connections. Obviously she can't do what you've done in the
test, as it uses the unexported function.
If this happens periodically, it can lead to serious resource leaks (the transport is
also preserved alive).
Am I missing something?

Status changed to New.

@rsc
Copy link
Contributor

rsc commented Sep 16, 2014

Comment 5:

Brad, can you look at Dmitriy's question?

Status changed to Accepted.

@gopherbot
Copy link

Comment 6:

CL https://golang.org/cl/148970043 mentions this issue.

@bradfitz
Copy link
Contributor

Comment 7:

This issue was closed by revision f13cec9.

Status changed to Fixed.

@rsc rsc added this to the Go1.4 milestone Apr 14, 2015
@rsc rsc removed the release-go1.4 label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jun 25, 2018
@bradfitz @wheatman
net/http: fix data race in test
2f55837 
I can't reproduce the race, but this should fix it.

Fixes golang#8483

LGTM=dvyukov
R=dvyukov
CC=golang-codereviews
https://golang.org/cl/126610043
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jun 25, 2018
@bradfitz @wheatman
net/http: make Transport.CloseIdleConnections also close pending dials
74692aa 
See comment 4 of https://code.google.com/p/go/issues/detail?id=8483#c4:

"So if a user creates a http.Client, issues a bunch of
requests and then wants to shutdown it and all opened connections;
what is she intended to do? The report suggests that just waiting for
all pending requests and calling CloseIdleConnections won't do, as
there can be new racing connections. Obviously she can't do what
you've done in the test, as it uses the unexported function.  If this
happens periodically, it can lead to serious resource leaks (the
transport is also preserved alive).  Am I missing something?"

This CL tracks the user's intention to close all idle
connections (CloseIdleConnections sets it true; and making a
new request sets it false). If a pending dial finishes and
nobody wants it, before it's retained for a future caller, the
"wantIdle" bool is checked and it's closed if the user has
called CloseIdleConnections without a later call to make a new
request.

Fixes golang#8483

LGTM=adg
R=golang-codereviews, dvyukov, adg
CC=golang-codereviews, rsc
https://golang.org/cl/148970043
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jun 26, 2018
@bradfitz @wheatman
net/http: make Transport.CloseIdleConnections also close pending dials
ea20286 
See comment 4 of https://code.google.com/p/go/issues/detail?id=8483#c4:

"So if a user creates a http.Client, issues a bunch of
requests and then wants to shutdown it and all opened connections;
what is she intended to do? The report suggests that just waiting for
all pending requests and calling CloseIdleConnections won't do, as
there can be new racing connections. Obviously she can't do what
you've done in the test, as it uses the unexported function.  If this
happens periodically, it can lead to serious resource leaks (the
transport is also preserved alive).  Am I missing something?"

This CL tracks the user's intention to close all idle
connections (CloseIdleConnections sets it true; and making a
new request sets it false). If a pending dial finishes and
nobody wants it, before it's retained for a future caller, the
"wantIdle" bool is checked and it's closed if the user has
called CloseIdleConnections without a later call to make a new
request.

Fixes golang#8483

LGTM=adg
R=golang-codereviews, dvyukov, adg
CC=golang-codereviews, rsc
https://golang.org/cl/148970043
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jul 9, 2018
@bradfitz @wheatman
net/http: fix data race in test
51f3fd5 
I can't reproduce the race, but this should fix it.

Fixes golang#8483

LGTM=dvyukov
R=dvyukov
CC=golang-codereviews
https://golang.org/cl/126610043
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jul 9, 2018
@bradfitz @wheatman
net/http: make Transport.CloseIdleConnections also close pending dials
7f56205 
See comment 4 of https://code.google.com/p/go/issues/detail?id=8483#c4:

"So if a user creates a http.Client, issues a bunch of
requests and then wants to shutdown it and all opened connections;
what is she intended to do? The report suggests that just waiting for
all pending requests and calling CloseIdleConnections won't do, as
there can be new racing connections. Obviously she can't do what
you've done in the test, as it uses the unexported function.  If this
happens periodically, it can lead to serious resource leaks (the
transport is also preserved alive).  Am I missing something?"

This CL tracks the user's intention to close all idle
connections (CloseIdleConnections sets it true; and making a
new request sets it false). If a pending dial finishes and
nobody wants it, before it's retained for a future caller, the
"wantIdle" bool is checked and it's closed if the user has
called CloseIdleConnections without a later call to make a new
request.

Fixes golang#8483

LGTM=adg
R=golang-codereviews, dvyukov, adg
CC=golang-codereviews, rsc
https://golang.org/cl/148970043
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jul 30, 2018
@bradfitz @wheatman
net/http: make Transport.CloseIdleConnections also close pending dials
de20fdb 
See comment 4 of https://code.google.com/p/go/issues/detail?id=8483#c4:

"So if a user creates a http.Client, issues a bunch of
requests and then wants to shutdown it and all opened connections;
what is she intended to do? The report suggests that just waiting for
all pending requests and calling CloseIdleConnections won't do, as
there can be new racing connections. Obviously she can't do what
you've done in the test, as it uses the unexported function.  If this
happens periodically, it can lead to serious resource leaks (the
transport is also preserved alive).  Am I missing something?"

This CL tracks the user's intention to close all idle
connections (CloseIdleConnections sets it true; and making a
new request sets it false). If a pending dial finishes and
nobody wants it, before it's retained for a future caller, the
"wantIdle" bool is checked and it's closed if the user has
called CloseIdleConnections without a later call to make a new
request.

Fixes golang#8483

LGTM=adg
R=golang-codereviews, dvyukov, adg
CC=golang-codereviews, rsc
https://golang.org/cl/148970043
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.4
Development

No branches or pull requests
4 participants
@bradfitz @rsc @dvyukov @gopherbot