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

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

Open
shinny-chengzhi opened this issue Sep 27, 2021 · 8 comments · May be fixed by #53118
Open

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

shinny-chengzhi opened this issue Sep 27, 2021 · 8 comments · May be fixed by #53118
Labels
NeedsFix
Milestone

Comments

@shinny-chengzhi
Copy link

@shinny-chengzhi shinny-chengzhi commented Sep 27, 2021

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.

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 3, 2021

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

@mknyszek mknyszek added the NeedsInvestigation label Oct 4, 2021
@mknyszek mknyszek added this to the Backlog milestone Oct 4, 2021
@mknyszek
Copy link
Contributor

@mknyszek mknyszek commented Oct 4, 2021

CC @neild

AlexanderYastrebov added a commit to AlexanderYastrebov/go that referenced this issue Oct 7, 2021
@dmitshur dmitshur added NeedsFix and removed NeedsInvestigation labels May 8, 2022
@dmitshur dmitshur removed this from the Backlog milestone May 8, 2022
@dmitshur dmitshur added this to the Go1.19 milestone May 8, 2022
@AlexanderYastrebov
Copy link
Contributor

@AlexanderYastrebov AlexanderYastrebov commented May 10, 2022

@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

@gopherbot gopherbot commented May 10, 2022

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
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
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
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
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

@ianlancetaylor ianlancetaylor commented May 11, 2022

Fix is being reverted.

gopherbot pushed a commit that referenced this issue May 11, 2022
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
Previous attempt https://golang.org/cl/353714 also incorrectly closed
accepted connection while in shutdown.

Fixes golang#48642
Updates golang#33313, golang#36819
@gopherbot
Copy link

@gopherbot gopherbot commented May 27, 2022

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

@gopherbot
Copy link

@gopherbot gopherbot commented May 31, 2022

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

@ianlancetaylor ianlancetaylor commented Jun 24, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix
Projects
Status: No status
6 participants