net/http/httptest: race in Close #51799

maisem · 2022-03-18T22:54:08Z

#304829 added logic to ensure that user ConnState callbacks complete before (*httptest.Server).Close returns.

The change however causes a race, if the order of events is

(*httptest.Server).Close() has already been called and is currently waiting for connections to close
a connection state changes which results in the ConnState callback to concurrently call (sync.WaitGroup).Add(1)

// 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()

When running tests with -race flag this is easily reproducible in our tests and is causing our CI to be flaky.

==================
WARNING: DATA RACE
Write at 0x00c0000cf628 by goroutine 36:
  runtime.racewrite()
      <autogenerated>:1 +0x10
  net/http/httptest.(*Server).Close()
      /Users/maisem/dev/tailscale-go/src/net/http/httptest/server.go:249 +0x1ac
  tailscale.io/control/controlclient.(*server).close()
      /Users/maisem/dev/tailscale.io/control/controlclient/auto_test.go:2046 +0x354
  tailscale.io/control/controlclient.TestControlRegisterEphemeral.func1()
      /Users/maisem/dev/tailscale.io/control/controlclient/auto_test.go:1816 +0x38
  runtime.deferreturn()
      /Users/maisem/dev/tailscale-go/src/runtime/panic.go:436 +0x34
  testing.tRunner()
      /Users/maisem/dev/tailscale-go/src/testing/testing.go:1439 +0x18c
  testing.(*T).Run.func1()
      /Users/maisem/dev/tailscale-go/src/testing/testing.go:1486 +0x44

Previous read at 0x00c0000cf628 by goroutine 52:
  runtime.raceread()
      <autogenerated>:1 +0x10
  net/http/httptest.(*Server).wrap.func1()
      /Users/maisem/dev/tailscale-go/src/net/http/httptest/server.go:323 +0x9c
  golang.org/x/net/http2.(*serverConn).setConnState()
      /Users/maisem/go/pkg/mod/golang.org/x/net@v0.0.0-20220127200216-cd36cc0744dd/http2/server.go:633 +0x2a8
  golang.org/x/net/http2.(*serverConn).closeStream()
      /Users/maisem/go/pkg/mod/golang.org/x/net@v0.0.0-20220127200216-cd36cc0744dd/http2/server.go:1540 +0x218
  golang.org/x/net/http2.(*serverConn).processResetStream()
      /Users/maisem/go/pkg/mod/golang.org/x/net@v0.0.0-20220127200216-cd36cc0744dd/http2/server.go:1519 +0x118
  golang.org/x/net/http2.(*serverConn).processFrame()
      /Users/maisem/go/pkg/mod/golang.org/x/net@v0.0.0-20220127200216-cd36cc0744dd/http2/server.go:1436 +0x1ec
  golang.org/x/net/http2.(*serverConn).processFrameFromReader()
      /Users/maisem/go/pkg/mod/golang.org/x/net@v0.0.0-20220127200216-cd36cc0744dd/http2/server.go:1386 +0x274
  golang.org/x/net/http2.(*serverConn).serve()
      /Users/maisem/go/pkg/mod/golang.org/x/net@v0.0.0-20220127200216-cd36cc0744dd/http2/server.go:885 +0xf84
  golang.org/x/net/http2.(*Server).ServeConn()
      /Users/maisem/go/pkg/mod/golang.org/x/net@v0.0.0-20220127200216-cd36cc0744dd/http2/server.go:471 +0xed8

This patches fixes the issue

diff --git a/src/net/http/httptest/server.go b/src/net/http/httptest/server.go
index 4f85ff55d8..3aebca810a 100644
--- a/src/net/http/httptest/server.go
+++ b/src/net/http/httptest/server.go
@@ -317,12 +317,6 @@ func (s *Server) wrap() {
                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 @@ func (s *Server) wrap() {
                                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 @@ func (s *Server) closeConnChan(c net.Conn, done chan<- struct{}) {
                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()
-       }
-}

cc @neild @bradfitz @josharian

The text was updated successfully, but these errors were encountered:

Fixes golang#51799 Signed-off-by: Maisem Ali <maisem@tailscale.com>

Related to golang#51799 Signed-off-by: Maisem Ali <maisem@tailscale.com>

Related to golang#51799 Fixes tailscale/corp#4393 Signed-off-by: Maisem Ali <maisem@tailscale.com>

Fixes golang#51799 Signed-off-by: Maisem Ali <maisem@tailscale.com>

Fixes golang#51799

When run with race detector the test fails without the fix. Fixes golang#51799

gopherbot · 2022-03-19T06:07:54Z

Change https://go.dev/cl/393974 mentions this issue: net/http/httptest: fix race in Close()

When run with race detector the test fails without the fix. Fixes golang#51799

rsc · 2022-03-23T17:04:19Z

@gopherbot please open backports for Go 1.18

gopherbot · 2022-03-23T17:05:55Z

Backport issue(s) opened: #51897 (for 1.18).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

maisem · 2022-03-23T19:10:12Z

@rsc fwiw the original change was cherry-picked on 1.16 so I'm almost sure this is also a problem in 1.17, but I haven't tested it on those versions yet.
https://go-review.googlesource.com/c/go/+/367516

maisem · 2022-03-23T19:19:39Z

Verified that the newly added test in #51805 also fails on release-branch.go1.17

maisem · 2022-03-23T19:21:47Z

also fails on release-branch.go1.16

There's a race condition in the h2_bundle.go of go 1.17 and later. The issue golang/go#51799 mentions that this might be fixed in go 1.19, so we'll need to wait for that. We make sure we build our docker images with go 1.16.8 to not run into the issue in the wild.

There's a race condition in the h2_bundle.go of go 1.16.10 and later. The issue golang/go#51799 mentions that this might be fixed in go 1.19, so we'll need to wait for that. We make sure we build our docker images with go 1.16.9 to not run into the issue in the wild.

When run with race detector the test fails without the fix. Fixes golang#51799 Change-Id: I273adb6d3a2b1e0d606b9c27ab4c6a9aa4aa8064 GitHub-Last-Rev: a5ddd14 GitHub-Pull-Request: golang#51805 Reviewed-on: https://go-review.googlesource.com/c/go/+/393974 Reviewed-by: Damien Neil <dneil@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Trust: Brad Fitzpatrick <bradfitz@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> (cherry picked from commit 1d19cea)

heschi · 2022-03-30T16:14:08Z

Note that we don't have a backport for 1.17, and #51897 is waiting for a cherrypick CL to be created.

When run with race detector the test fails without the fix. Fixes golang#51799 Change-Id: I273adb6d3a2b1e0d606b9c27ab4c6a9aa4aa8064 GitHub-Last-Rev: a5ddd14 GitHub-Pull-Request: golang#51805 Reviewed-on: https://go-review.googlesource.com/c/go/+/393974 Reviewed-by: Damien Neil <dneil@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Trust: Brad Fitzpatrick <bradfitz@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> (cherry picked from commit 1d19cea)

neild · 2022-04-20T19:10:02Z

@gopherbot please open backport for Go 1.17

Backporting to 1.18 seems reasonable, and if we're doing that we should do 1.17 as well.

Edit: Oh right, gopherbot only acts once on backport requests. Creating manually.

gopherbot · 2022-04-20T19:11:25Z

Change https://go.dev/cl/401317 mentions this issue: [release-branch.go1.18] net/http/httptest: fix race in Server.Close

gopherbot · 2022-04-20T19:14:09Z

Change https://go.dev/cl/401318 mentions this issue: [release-branch.go1.17] net/http/httptest: fix race in Server.Close

When run with race detector the test fails without the fix. For #51799 Fixes #52455 Change-Id: I273adb6d3a2b1e0d606b9c27ab4c6a9aa4aa8064 GitHub-Last-Rev: a5ddd14 GitHub-Pull-Request: #51805 Reviewed-on: https://go-review.googlesource.com/c/go/+/393974 Reviewed-by: Damien Neil <dneil@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Trust: Brad Fitzpatrick <bradfitz@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> (cherry picked from commit 1d19cea) Reviewed-on: https://go-review.googlesource.com/c/go/+/401318 Reviewed-by: David Chase <drchase@google.com>

When run with race detector the test fails without the fix. For #51799 Fixes #51897 Change-Id: I273adb6d3a2b1e0d606b9c27ab4c6a9aa4aa8064 GitHub-Last-Rev: a5ddd14 GitHub-Pull-Request: #51805 Reviewed-on: https://go-review.googlesource.com/c/go/+/393974 Reviewed-by: Damien Neil <dneil@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Trust: Brad Fitzpatrick <bradfitz@golang.org> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> (cherry picked from commit 1d19cea) Reviewed-on: https://go-review.googlesource.com/c/go/+/401317 Reviewed-by: David Chase <drchase@google.com>

josharian changed the title ~~httptest: race in Close()~~ net/http/httptest: race in Close Mar 18, 2022

maisem added a commit to tailscale/go that referenced this issue Mar 18, 2022

net/http/httptest: fix race in Close()

deb434e

Fixes golang#51799 Signed-off-by: Maisem Ali <maisem@tailscale.com>

maisem added a commit to tailscale/go that referenced this issue Mar 18, 2022

net/http/httptest: fix race in Close()

fdf5e1e

Fixes golang#51799 Signed-off-by: Maisem Ali <maisem@tailscale.com>

maisem added a commit to tailscale/go that referenced this issue Mar 18, 2022

net/http/httptest: fix race in Close()

48949c3

Related to golang#51799 Signed-off-by: Maisem Ali <maisem@tailscale.com>

maisem mentioned this issue Mar 18, 2022

net/http/httptest: fix race in Close() tailscale/go#30

Closed

maisem added a commit to tailscale/go that referenced this issue Mar 18, 2022

net/http/httptest: fix race in Close()

5c0ab4e

Related to golang#51799 Fixes tailscale/corp#4393 Signed-off-by: Maisem Ali <maisem@tailscale.com>

maisem added a commit to maisem/go that referenced this issue Mar 19, 2022

net/http/httptest: fix race in Close()

5527c25

Fixes golang#51799 Signed-off-by: Maisem Ali <maisem@tailscale.com>

maisem added a commit to maisem/go that referenced this issue Mar 19, 2022

net/http/httptest: fix race in Close()

466e755

Fixes golang#51799

maisem added a commit to maisem/go that referenced this issue Mar 19, 2022

net/http/httptest: fix race in Close()

8c5c255

When run with race detector the test fails without the fix. Fixes golang#51799

maisem mentioned this issue Mar 19, 2022

net/http/httptest: fix race in Server.Close #51805

Closed

ianlancetaylor added RaceDetector NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Mar 19, 2022

ianlancetaylor added this to the Backlog milestone Mar 19, 2022

maisem added a commit to maisem/go that referenced this issue Mar 19, 2022

net/http/httptest: fix race in Close

7e8e6ce

When run with race detector the test fails without the fix. Fixes golang#51799

maisem added a commit to maisem/go that referenced this issue Mar 19, 2022

net/http/httptest: fix race in Close

21cf6ba

When run with race detector the test fails without the fix. Fixes golang#51799

maisem added a commit to maisem/go that referenced this issue Mar 19, 2022

net/http/httptest: fix race in Close

497a1c1

When run with race detector the test fails without the fix. Fixes golang#51799

maisem added a commit to maisem/go that referenced this issue Mar 21, 2022

net/http/httptest: fix race in Server.Close

a5ddd14

When run with race detector the test fails without the fix. Fixes golang#51799

rsc added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 23, 2022

rsc modified the milestones: Backlog, Go1.19 Mar 23, 2022

gopherbot mentioned this issue Mar 23, 2022

net/http/httptest: race in Close [1.18 backport] #51897

Closed

gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 23, 2022

liggitt mentioned this issue Mar 24, 2022

go1.18: Data race: client-go retry body reset kubernetes/kubernetes#108906

Closed

gopherbot closed this as completed in 1d19cea Mar 25, 2022

maisem mentioned this issue Mar 25, 2022

net/http/httptest: fix race in Server.Close tailscale/go#31

Merged

neild mentioned this issue Apr 20, 2022

net/http/httptest: race in Close [1.17 backport] #52455

Closed

aojea mentioned this issue May 11, 2022

integration: force close httpserver on exit kubernetes/kubernetes#109901

Merged

golang locked and limited conversation to collaborators Apr 20, 2023

gopherbot added the FrozenDueToAge label Apr 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net/http/httptest: race in Close #51799

net/http/httptest: race in Close #51799

maisem commented Mar 18, 2022

gopherbot commented Mar 19, 2022

rsc commented Mar 23, 2022

gopherbot commented Mar 23, 2022

maisem commented Mar 23, 2022

maisem commented Mar 23, 2022

maisem commented Mar 23, 2022

heschi commented Mar 30, 2022

neild commented Apr 20, 2022 •

edited

gopherbot commented Apr 20, 2022

gopherbot commented Apr 20, 2022

net/http/httptest: race in Close #51799

net/http/httptest: race in Close #51799

Comments

maisem commented Mar 18, 2022

gopherbot commented Mar 19, 2022

rsc commented Mar 23, 2022

gopherbot commented Mar 23, 2022

maisem commented Mar 23, 2022

maisem commented Mar 23, 2022

maisem commented Mar 23, 2022

heschi commented Mar 30, 2022

neild commented Apr 20, 2022 • edited

gopherbot commented Apr 20, 2022

gopherbot commented Apr 20, 2022

neild commented Apr 20, 2022 •

edited