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: Server.Close may not able to close all connections #48642

Closed
shinny-chengzhi opened this issue Sep 27, 2021 · 8 comments
Closed

net/http: Server.Close may not able to close all connections #48642

shinny-chengzhi opened this issue Sep 27, 2021 · 8 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.19

Comments

@shinny-chengzhi
Copy link

I encountered lingering connection after calling Server.Close and it's not closed by Server.Close

Here is reproducible example: https://play.golang.org/p/9aLVDwWPQhH

The problem is http/server.go: Server.Serve function didn't check if Server is already closed before calling handler, and if Server.Close been called after l.Accept return but before c.setState(c.rwc, StateNew, runHooks), the new connection is not registered in the activeConn map when Server.Close try to close all connections.

The example code simulate this condition by sleep one second during ConnContext call. Program can exit corrected if this sleep is removed.
AlexanderYastrebov added a commit to AlexanderYastrebov/go that referenced this issue Oct 3, 2021
@AlexanderYastrebov
net/http: close accepted connection
2b23f0f 
Fixes golang#48642
@AlexanderYastrebov AlexanderYastrebov mentioned this issue Oct 3, 2021
Closed
@gopherbot
Copy link

Change https://golang.org/cl/353714 mentions this issue: net/http: close accepted connection

@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 4, 2021
@mknyszek mknyszek added this to the Backlog milestone Oct 4, 2021
@mknyszek
Copy link
Contributor

mknyszek commented Oct 4, 2021

CC @neild

AlexanderYastrebov added a commit to AlexanderYastrebov/go that referenced this issue Oct 7, 2021
@AlexanderYastrebov
net/http: reject connections accepted after Server.Close
27db2ba 
Fixes golang#48642
This was referenced Oct 17, 2021
Closed
Open
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 8, 2022
@dmitshur dmitshur modified the milestones: Backlog, Go1.19 May 8, 2022
@AlexanderYastrebov AlexanderYastrebov mentioned this issue May 10, 2022
Closed
@AlexanderYastrebov
Copy link
Contributor

@neild

I think this was not the correct fix.
The change closes accepted connection also in graceful shutdown which
breaks the fix for #33313 (and apparent duplicate #36819).

The proper fix should close accepted connection only if server is closed
but not in graceful shutdown.

As I am not sure what is the correct way to revert the change I've created a #52823

@gopherbot
Copy link

Change https://go.dev/cl/405454 mentions this issue: Revert "net/http: close accepted connection"

AlexanderYastrebov added a commit to AlexanderYastrebov/go that referenced this issue May 11, 2022
@AlexanderYastrebov
net/http: do not close accepted connection during graceful shutdown
d74b231 
The https://golang.org/cl/353714 wrongly closes accepted connection
if server is shutting down.

Updates golang#33313
Fixes golang#36819
Fixes golang#48642
AlexanderYastrebov added a commit to AlexanderYastrebov/go that referenced this issue May 11, 2022
@AlexanderYastrebov
net/http: do not close accepted connection during graceful shutdown
91e8539 
The https://golang.org/cl/353714 wrongly closes accepted connection
if server is shutting down.

Updates golang#33313
Fixes golang#36819
Fixes golang#48642
AlexanderYastrebov added a commit to AlexanderYastrebov/go that referenced this issue May 11, 2022
@AlexanderYastrebov
net/http: do not close accepted connection during graceful shutdown
abd0a9e 
The https://golang.org/cl/353714 wrongly closes accepted connection
if server is shutting down.

Updates golang#33313
Fixes golang#36819
Fixes golang#48642
AlexanderYastrebov added a commit to AlexanderYastrebov/go that referenced this issue May 11, 2022
@AlexanderYastrebov
net/http: do not close accepted connection during graceful shutdown
b498520 
The https://golang.org/cl/353714 wrongly closes accepted connection
if server is shutting down.

Updates golang#33313
Fixes golang#36819
Fixes golang#48642
@ianlancetaylor
Copy link
Contributor

Fix is being reverted.

gopherbot pushed a commit that referenced this issue May 11, 2022
@AlexanderYastrebov @ianlancetaylor
Revert "net/http: close accepted connection"
c14ed5b 
This reverts CL 353714.

The change closes accepted connection also in graceful shutdown which
breaks the fix for #33313 (and apparent duplicate #36819).

The proper fix should close accepted connection only if server is closed
but not in graceful shutdown.

Updates #48642

Change-Id: I2f7005f3f3037e6563745731bb2693923b654004
GitHub-Last-Rev: f6d885a
GitHub-Pull-Request: #52823
Reviewed-on: https://go-review.googlesource.com/c/go/+/405454
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
AlexanderYastrebov added a commit to AlexanderYastrebov/go that referenced this issue May 27, 2022
@AlexanderYastrebov
net/http: close accepted connection when Closed
6d2d9e3 
Previous attempt https://golang.org/cl/353714 also incorrectly closed
accepted connection while in shutdown.

Fixes golang#48642
Updates golang#33313, golang#36819
@AlexanderYastrebov AlexanderYastrebov mentioned this issue May 27, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/409154 mentions this issue: net/http: close accepted connection when Closed

@gopherbot
Copy link

Change https://go.dev/cl/409537 mentions this issue: net/http: wait for listeners to exit in Server.Close and Shutdown

@ianlancetaylor
Copy link
Contributor

@neild What is the status of this issue? It's currently in the 1.19 milestone. Thanks.

jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
@neild @jproberts
net/http: wait for listeners to exit in Server.Close and Shutdown
886ab38 
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 golang#48642
Updates golang#33313, golang#36819

Change-Id: I109a93362680991bf298e0a95637595dcaa884af
Reviewed-on: https://go-review.googlesource.com/c/go/+/409537
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
@golang golang locked and limited conversation to collaborators Jul 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
Release Status & Release Blockers
Status: Done
Milestone
Go1.19
6 participants
@AlexanderYastrebov @mknyszek @dmitshur @ianlancetaylor @gopherbot @shinny-chengzhi