[go] net/http: wait for listeners to exit in Server.Close and Shutdown
Damien Neil (Gerrit)
Damien Neil has uploaded this change for review.
net/http: wait for listeners to exit in Server.Close and Shutdown
Avoid race conditions when a new connection is accepted just after
Server.Close or Server.Shutdown is called by waiting for the
listener goroutines to exit before proceeding to clean up active
connections.
No test because the mechanism required to trigger the race condition
reliably requires such tight coupling to the Server internals that
any test would be quite fragile in the face of reasonable refactorings.
Fixes #48642
Updates #33313, #36819
Change-Id: I109a93362680991bf298e0a95637595dcaa884af
---
M src/net/http/server.go
1 file changed, 31 insertions(+), 0 deletions(-)
diff --git a/src/net/http/server.go b/src/net/http/server.go
index bc3a463..3611e9f 100644
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -2690,6 +2690,8 @@
activeConn map[*conn]struct{}
doneChan chan struct{}
onShutdown []func()
+
+ listenerGroup sync.WaitGroup
}
func (s *Server) getDoneChan() <-chan struct{} {
@@ -2732,6 +2734,11 @@
defer srv.mu.Unlock()
srv.closeDoneChanLocked()
err := srv.closeListenersLocked()
+
+ srv.mu.Unlock()
+ srv.listenerGroup.Wait()
+ srv.mu.Lock()
+
for c := range srv.activeConn {
c.rwc.Close()
delete(srv.activeConn, c)
@@ -2778,6 +2785,7 @@
go f()
}
srv.mu.Unlock()
+ srv.listenerGroup.Wait()
pollIntervalBase := time.Millisecond
nextPollInterval := func() time.Duration {
@@ -3157,8 +3165,10 @@
return false
}
s.listeners[ln] = struct{}{}
+ s.listenerGroup.Add(1)
} else {
delete(s.listeners, ln)
+ s.listenerGroup.Done()
}
return true
}
To view, visit change 409537. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Patch set 1:Run-TryBot +1
Ivan Kurnosov (Gerrit)
Attention is currently required from: Damien Neil.
2 comments:
Patchset:
I think with this change it's possible to have a race between wg's Wait and Add calls.
File src/net/http/server.go:
Patch Set #1, Line 3168:
s.listenerGroup.Add(1)Is this call not in race with two calls above to wait? `srv.listenerGroup.Wait()`?
To view, visit change 409537. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Ivan Kurnosov.
1 comment:
File src/net/http/server.go:
Patch Set #1, Line 3168:
s.listenerGroup.Add(1)
Is this call not in race with two calls above to wait? `srv.listenerGroup. […]
Add is only called if srv.inShutdown is not set, and srv.mu synchronizes access to srv.inShutdown between the Add and Wait calls.
To view, visit change 409537. To unsubscribe, or for help writing mail filters, visit settings.
Ivan Kurnosov (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Damien Neil.
2 comments:
Patchset:
Right, I see now, thanks (PS: I'm not sure how to make my code comment as "Resolved" hopefully this comment marks it as such).
File src/net/http/server.go:
Patch Set #1, Line 3168:
s.listenerGroup.Add(1)
Add is only called if srv.inShutdown is not set, and srv.mu synchronizes access to srv. […]
Ack
To view, visit change 409537. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Yastrebov (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Damien Neil.
1 comment:
File src/net/http/server.go:
Patch Set #1, Line 2788:
srv.listenerGroup.Wait()This apparently allows to undo the https://golang.org/cl/213442
To view, visit change 409537. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Alexander Yastrebov, Brad Fitzpatrick.
1 comment:
File src/net/http/server.go:
Patch Set #1, Line 2788:
srv.listenerGroup.Wait()
This apparently allows to undo the https://golang. […]
How is that? I'm not seeing it, but I'm probably missing something.
To view, visit change 409537. To unsubscribe, or for help writing mail filters, visit settings.
Alexander Yastrebov (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Damien Neil.
1 comment:
File src/net/http/server.go:
Patch Set #1, Line 2788:
srv.listenerGroup.Wait()
How is that? I'm not seeing it, but I'm probably missing something.
It changed shutdown to wait for all listeners to be untracked (delete(s.listeners)) via defer in Serve.
The listenerGroup barrier ensures the same.
To view, visit change 409537. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Brad Fitzpatrick, Damien Neil.
Damien Neil uploaded patch set #2 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Damien Neil, TryBot-Result+1 by Gopher Robot
net/http: wait for listeners to exit in Server.Close and Shutdown
Avoid race conditions when a new connection is accepted just after
Server.Close or Server.Shutdown is called by waiting for the
listener goroutines to exit before proceeding to clean up active
connections.
No test because the mechanism required to trigger the race condition
reliably requires such tight coupling to the Server internals that
any test would be quite fragile in the face of reasonable refactorings.
Fixes #48642
Updates #33313, #36819
Change-Id: I109a93362680991bf298e0a95637595dcaa884af
---
M src/net/http/server.go
1 file changed, 32 insertions(+), 7 deletions(-)
To view, visit change 409537. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Alexander Yastrebov, Brad Fitzpatrick.
Patch set 1:Run-TryBot +1
1 comment:
File src/net/http/server.go:
Patch Set #1, Line 2788:
srv.listenerGroup.Wait()
It changed shutdown to wait for all listeners to be untracked (delete(s. […]
Ah, I misunderstood what you were saying. Good point.
To view, visit change 409537. To unsubscribe, or for help writing mail filters, visit settings.
Brad Fitzpatrick (Gerrit)
Attention is currently required from: Alexander Yastrebov, Damien Neil.
Patch set 2:Code-Review +2
3 comments:
File src/net/http/server.go:
Patch Set #2, Line 2738:
srv.mu.Unlock()This looks like a mistake at first glance. Might be worth a comment so reader knows that the unlock-and-re-lock is intentional.
Patch Set #2, Line 2738:
srv.mu.Unlock()Actually what's this letting happen? What's the Unlock for? For trackListener to able to run I guess? Leave a comment about that?
Patch Set #2, Line 3158:
if s.shuttingDown() {And I guess this is the bit that prevents the Add(1) a few lines below from panicking on concurrent WaitGroup.Wait?
To view, visit change 409537. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Alexander Yastrebov, Damien Neil.
Damien Neil uploaded patch set #3 to this change.
net/http: wait for listeners to exit in Server.Close and Shutdown
Avoid race conditions when a new connection is accepted just after
Server.Close or Server.Shutdown is called by waiting for the
listener goroutines to exit before proceeding to clean up active
connections.
No test because the mechanism required to trigger the race condition
reliably requires such tight coupling to the Server internals that
any test would be quite fragile in the face of reasonable refactorings.
Fixes #48642
Updates #33313, #36819
Change-Id: I109a93362680991bf298e0a95637595dcaa884af
---
M src/net/http/server.go
1 file changed, 36 insertions(+), 7 deletions(-)
To view, visit change 409537. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Alexander Yastrebov.
3 comments:
File src/net/http/server.go:
Patch Set #2, Line 2738:
srv.mu.Unlock()
This looks like a mistake at first glance. […]
Done
Patch Set #2, Line 2738:
srv.mu.Unlock()
Actually what's this letting happen? What's the Unlock for? For trackListener to able to run I guess […]
Added some more comments here.
Patch Set #2, Line 3158:
if s.shuttingDown() {
And I guess this is the bit that prevents the Add(1) a few lines below from panicking on concurrent […]
Yes, exactly.
To view, visit change 409537. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Alexander Yastrebov, Damien Neil.
Patch set 3:Code-Review +1
Damien Neil (Gerrit)
Damien Neil submitted this change.
2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/net/http/server.go
Insertions: 4, Deletions: 0.
The diff is too large to show. Please review the diff.
```
net/http: wait for listeners to exit in Server.Close and Shutdown
Avoid race conditions when a new connection is accepted just after
Server.Close or Server.Shutdown is called by waiting for the
listener goroutines to exit before proceeding to clean up active
connections.
No test because the mechanism required to trigger the race condition
reliably requires such tight coupling to the Server internals that
any test would be quite fragile in the face of reasonable refactorings.
Fixes #48642
Updates #33313, #36819
Change-Id: I109a93362680991bf298e0a95637595dcaa884af
Reviewed-on: https://go-review.googlesource.com/c/go/+/409537
TryBot-Result: Gopher Robot <go...@golang.org>
Run-TryBot: Damien Neil <dn...@google.com>
Reviewed-by: Brad Fitzpatrick <brad...@golang.org>
Reviewed-by: Bryan Mills <bcm...@google.com>
---
M src/net/http/server.go
1 file changed, 41 insertions(+), 7 deletions(-)
diff --git a/src/net/http/server.go b/src/net/http/server.go
index bc3a463..87dd412 100644
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -2690,6 +2690,8 @@
activeConn map[*conn]struct{}
doneChan chan struct{}
onShutdown []func()
+
+ listenerGroup sync.WaitGroup
}
func (s *Server) getDoneChan() <-chan struct{} {
@@ -2732,6 +2734,15 @@
defer srv.mu.Unlock()
srv.closeDoneChanLocked()
err := srv.closeListenersLocked()
+
+ // Unlock srv.mu while waiting for listenerGroup.
+ // The group Add and Done calls are made with srv.mu held,
+ // to avoid adding a new listener in the window between
+ // us setting inShutdown above and waiting here.
+ srv.mu.Unlock()
+ srv.listenerGroup.Wait()
+ srv.mu.Lock()
+
for c := range srv.activeConn {
c.rwc.Close()
delete(srv.activeConn, c)
@@ -2778,6 +2789,7 @@
go f()
}
srv.mu.Unlock()
+ srv.listenerGroup.Wait()
pollIntervalBase := time.Millisecond
nextPollInterval := func() time.Duration {
@@ -2794,7 +2806,7 @@
timer := time.NewTimer(nextPollInterval())
defer timer.Stop()
for {
- if srv.closeIdleConns() && srv.numListeners() == 0 {
+ if srv.closeIdleConns() {
return lnerr
}
select {
@@ -2817,12 +2829,6 @@
srv.mu.Unlock()
}
-func (s *Server) numListeners() int {
- s.mu.Lock()
- defer s.mu.Unlock()
- return len(s.listeners)
-}
-
// closeIdleConns closes all idle connections and reports whether the
// server is quiescent.
func (s *Server) closeIdleConns() bool {
@@ -3157,8 +3163,10 @@
return false
}
s.listeners[ln] = struct{}{}
+ s.listenerGroup.Add(1)
} else {
delete(s.listeners, ln)
+ s.listenerGroup.Done()
}
return true
}
To view, visit change 409537. To unsubscribe, or for help writing mail filters, visit settings.