[go] net/http/httptest: fix race in Close()
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded this change for review.
net/http/httptest: fix race in Close()
When run with race detector the test fails without the fix.
Fixes #51799
/cc @bradfitz @josharian
Change-Id: I273adb6d3a2b1e0d606b9c27ab4c6a9aa4aa8064
GitHub-Last-Rev: 8c5c255bbf81ab0542aaf101c69adc64097cbfee
GitHub-Pull-Request: golang/go#51805
---
M src/net/http/httptest/server.go
M src/net/http/httptest/server_test.go
2 files changed, 77 insertions(+), 17 deletions(-)
diff --git a/src/net/http/httptest/server.go b/src/net/http/httptest/server.go
index 4f85ff5..3aebca8 100644
--- a/src/net/http/httptest/server.go
+++ b/src/net/http/httptest/server.go
@@ -317,12 +317,6 @@
s.mu.Lock()
defer s.mu.Unlock()
- // Keep Close from returning until the user's ConnState hook
- // (if any) finishes. Without this, the call to forgetConn
- // below might send the count to 0 before we run the hook.
- s.wg.Add(1)
- defer s.wg.Done()
-
switch cs {
case http.StateNew:
s.wg.Add(1)
@@ -358,7 +352,12 @@
s.closeConn(c)
}
case http.StateHijacked, http.StateClosed:
- s.forgetConn(c)
+ if _, ok := s.conns[c]; ok {
+ delete(s.conns, c)
+ // Keep Close from returning until the user's ConnState hook
+ // (if any) finishes.
+ defer s.wg.Done()
+ }
}
if oldHook != nil {
oldHook(c, cs)
@@ -378,13 +377,3 @@
done <- struct{}{}
}
}
-
-// forgetConn removes c from the set of tracked conns and decrements it from the
-// waitgroup, unless it was previously removed.
-// s.mu must be held.
-func (s *Server) forgetConn(c net.Conn) {
- if _, ok := s.conns[c]; ok {
- delete(s.conns, c)
- s.wg.Done()
- }
-}
diff --git a/src/net/http/httptest/server_test.go b/src/net/http/httptest/server_test.go
index 39568b3..5313f65 100644
--- a/src/net/http/httptest/server_test.go
+++ b/src/net/http/httptest/server_test.go
@@ -9,6 +9,7 @@
"io"
"net"
"net/http"
+ "sync"
"testing"
)
@@ -203,6 +204,59 @@
ts.Close() // tests that it doesn't panic
}
+// Issue 51799: test hijacking a connection and then closing it
+// concurrently with closing the server.
+func TestCloseHijackedConnection(t *testing.T) {
+ hijacked := make(chan net.Conn)
+ ts := NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ defer close(hijacked)
+ hj, ok := w.(http.Hijacker)
+ if !ok {
+ t.Fatal("failed to hijack")
+ }
+ c, _, err := hj.Hijack()
+ if err != nil {
+ t.Fatal(err)
+ }
+ hijacked <- c
+ }))
+
+ var wg sync.WaitGroup
+ wg.Add(1)
+ go func() {
+ defer wg.Done()
+ req, err := http.NewRequest("GET", ts.URL, nil)
+ if err != nil {
+ t.Log(err)
+ }
+ // Use a client not associated with the Server.
+ var c http.Client
+ resp, err := c.Do(req)
+ if err != nil {
+ t.Log(err)
+ return
+ }
+ resp.Body.Close()
+ }()
+
+ wg.Add(1)
+ conn := <-hijacked
+ go func(conn net.Conn) {
+ defer wg.Done()
+ // Close the connection and then inform the Server that
+ // we closed it.
+ conn.Close()
+ ts.Config.ConnState(conn, http.StateClosed)
+ }(conn)
+
+ wg.Add(1)
+ go func() {
+ defer wg.Done()
+ ts.Close()
+ }()
+ wg.Wait()
+}
+
func TestTLSServerWithHTTP2(t *testing.T) {
modes := []struct {
name string
To view, visit change 393974. To unsubscribe, or for help writing mail filters, visit settings.
Gopher Robot (Gerrit)
Congratulations on opening your first change. Thank you for your contribution!
Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11 or adds a tag like "wait-release", it means that this CL will be
reviewed as part of the next development cycle. See https://go.dev/s/release
for more details.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Damien Neil.
3 comments:
Commit Message:
Patch Set #1, Line 7:
net/http/httptest: fix race in Close()Drop the parentheses. We don't usually use them.
Patch Set #1, Line 13:
/cc @bradfitz @josharianRemove this line, it doesn't belong in the change description.
File src/net/http/httptest/server.go:
Patch Set #1, Line 359:
defer s.wg.Done()Where is the s.wg.Add(1) that corresponds to this call to s.wg.Done?
To view, visit change 393974. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Damien Neil.
Gerrit Bot uploaded patch set #2 to this change.
net/http/httptest: fix race in Close
When run with race detector the test fails without the fix.
Fixes #51799
Change-Id: I273adb6d3a2b1e0d606b9c27ab4c6a9aa4aa8064
GitHub-Last-Rev: 7e8e6cef65ec6640e87c9c5449be7b809659e060
GitHub-Pull-Request: golang/go#51805
---
M src/net/http/httptest/server.go
M src/net/http/httptest/server_test.go
2 files changed, 75 insertions(+), 17 deletions(-)
To view, visit change 393974. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Damien Neil.
Gerrit Bot uploaded patch set #3 to this change.
net/http/httptest: fix race in Close
When run with race detector the test fails without the fix.
Fixes #51799
Change-Id: I273adb6d3a2b1e0d606b9c27ab4c6a9aa4aa8064
GitHub-Last-Rev: 21cf6ba3a9dabbbe30ce778909d0a0a012d28287
GitHub-Pull-Request: golang/go#51805
---
M src/net/http/httptest/server.go
M src/net/http/httptest/server_test.go
2 files changed, 80 insertions(+), 18 deletions(-)
To view, visit change 393974. To unsubscribe, or for help writing mail filters, visit settings.
Maisem Ali (Gerrit)
Attention is currently required from: Damien Neil, Ian Lance Taylor.
3 comments:
Commit Message:
Patch Set #1, Line 7:
net/http/httptest: fix race in Close()Drop the parentheses. We don't usually use them.
Done
Patch Set #1, Line 13:
/cc @bradfitz @josharianRemove this line, it doesn't belong in the change description.
I didn't realize how it would get copied over. Removed.
File src/net/http/httptest/server.go:
Patch Set #1, Line 359:
defer s.wg.Done()
Where is the s.wg.Add(1) that corresponds to this call to s.wg. […]
I accidentally lost the comment from forgetConn, added it back and also added a comment above
To view, visit change 393974. To unsubscribe, or for help writing mail filters, visit settings.
Maisem Ali (Gerrit)
Attention is currently required from: Damien Neil, Ian Lance Taylor.
1 comment:
File src/net/http/httptest/server.go:
Patch Set #1, Line 359:
defer s.wg.Done()
I accidentally lost the comment from forgetConn, added it back and also added a comment above
By which I mean the corresponding wg.Add is at line 330
To view, visit change 393974. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Damien Neil, Ian Lance Taylor.
Gerrit Bot uploaded patch set #4 to this change.
net/http/httptest: fix race in Close
When run with race detector the test fails without the fix.
Fixes #51799
Change-Id: I273adb6d3a2b1e0d606b9c27ab4c6a9aa4aa8064
GitHub-Last-Rev: 497a1c1d64bcabc269c2c382205f21b95229f215
GitHub-Pull-Request: golang/go#51805
---
M src/net/http/httptest/server.go
M src/net/http/httptest/server_test.go
2 files changed, 80 insertions(+), 18 deletions(-)
To view, visit change 393974. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
Attention is currently required from: Damien Neil, Ian Lance Taylor.
1 comment:
File src/net/http/httptest/server.go:
Patch Set #4, Line 363:
defer s.wg.Done()This defer isn't doing anything that I can see?
To view, visit change 393974. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
File src/net/http/httptest/server.go:
Patch Set #4, Line 363:
defer s.wg.Done()This defer isn't doing anything that I can see?
Ah, oldHook might be non-nil. Okay.
To view, visit change 393974. To unsubscribe, or for help writing mail filters, visit settings.
Maisem Ali (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Damien Neil, Ian Lance Taylor.
1 comment:
File src/net/http/httptest/server.go:
Patch Set #4, Line 363:
defer s.wg.Done()
Ah, oldHook might be non-nil. Okay.
Resolving
To view, visit change 393974. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Damien Neil.
Patch set 4:Run-TryBot +1
Brad Fitzpatrick (Gerrit)
Attention is currently required from: Damien Neil.
2 comments:
Commit Message:
Patch Set #4, Line 7:
net/http/httptest: fix race in Closein Server.Close
Patchset:
R=dneil
I was hoping Bryan could review this too, but I see his Gerrit name says he's OOO until March 30th.
Damien, can you review?
To view, visit change 393974. To unsubscribe, or for help writing mail filters, visit settings.
Maisem Ali (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Damien Neil.
1 comment:
Commit Message:
Patch Set #4, Line 7:
net/http/httptest: fix race in Close
in Server. […]
Done
To view, visit change 393974. To unsubscribe, or for help writing mail filters, visit settings.
Gerrit Bot (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Damien Neil.
Gerrit Bot uploaded patch set #5 to this change.
net/http/httptest: fix race in Server.Close
When run with race detector the test fails without the fix.
Fixes #51799
Change-Id: I273adb6d3a2b1e0d606b9c27ab4c6a9aa4aa8064
GitHub-Last-Rev: a5ddd146a2a65f2e817eed5133449c79b3af2562
GitHub-Pull-Request: golang/go#51805
---
M src/net/http/httptest/server.go
M src/net/http/httptest/server_test.go
2 files changed, 80 insertions(+), 18 deletions(-)
To view, visit change 393974. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Brad Fitzpatrick.
Patch set 5:Code-Review +2
Brad Fitzpatrick (Gerrit)
Patch set 5:Run-TryBot +1Code-Review +2Trust +1
Damien Neil (Gerrit)
Damien Neil submitted this change.
net/http/httptest: fix race in Server.Close
When run with race detector the test fails without the fix.
Fixes #51799
Change-Id: I273adb6d3a2b1e0d606b9c27ab4c6a9aa4aa8064
GitHub-Last-Rev: a5ddd146a2a65f2e817eed5133449c79b3af2562
GitHub-Pull-Request: golang/go#51805
Reviewed-on: https://go-review.googlesource.com/c/go/+/393974
Reviewed-by: Damien Neil <dn...@google.com>
Reviewed-by: Brad Fitzpatrick <brad...@golang.org>
Trust: Brad Fitzpatrick <brad...@golang.org>
Run-TryBot: Brad Fitzpatrick <brad...@golang.org>
---
M src/net/http/httptest/server.go
M src/net/http/httptest/server_test.go
2 files changed, 85 insertions(+), 18 deletions(-)
diff --git a/src/net/http/httptest/server.go b/src/net/http/httptest/server.go
index 4f85ff5..1c0c0f69 100644
--- a/src/net/http/httptest/server.go
+++ b/src/net/http/httptest/server.go
@@ -317,21 +317,17 @@
s.mu.Lock()
defer s.mu.Unlock()
- // Keep Close from returning until the user's ConnState hook
- // (if any) finishes. Without this, the call to forgetConn
- // below might send the count to 0 before we run the hook.
- s.wg.Add(1)
- defer s.wg.Done()
-
switch cs {
case http.StateNew:
- s.wg.Add(1)
if _, exists := s.conns[c]; exists {
panic("invalid state transition")
}
if s.conns == nil {
s.conns = make(map[net.Conn]http.ConnState)
}
+ // Add c to the set of tracked conns and increment it to the
+ // waitgroup.
+ s.wg.Add(1)
s.conns[c] = cs
if s.closed {
// Probably just a socket-late-binding dial from
@@ -358,7 +354,14 @@
s.closeConn(c)
}
case http.StateHijacked, http.StateClosed:
- s.forgetConn(c)
+ // Remove c from the set of tracked conns and decrement it from the
+ // waitgroup, unless it was previously removed.
+ if _, ok := s.conns[c]; ok {
+ delete(s.conns, c)
+ // Keep Close from returning until the user's ConnState hook
+ // (if any) finishes.
+ defer s.wg.Done()
+ }
}
if oldHook != nil {
oldHook(c, cs)
@@ -378,13 +381,3 @@To view, visit change 393974. To unsubscribe, or for help writing mail filters, visit settings.