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.

Sign up for GitHub

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: Client errors caused by Server.Shutdown should be always retryable #65802

Open
tocrafty opened this issue Feb 20, 2024 · 2 comments · May be fixed by #65805
Open

net/http: Client errors caused by Server.Shutdown should be always retryable #65802

tocrafty opened this issue Feb 20, 2024 · 2 comments · May be fixed by #65805
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@tocrafty
Copy link

Go version

go version go1.21.3 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/root/.cache/go-build'
GOENV='/root/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/root/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/root/go'
GOPRIVATE=''
GOPROXY='xxxxxx,direct'
GOROOT='/root/sdk/go1.21.3'
GOSUMDB='xxxx'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/root/sdk/go1.21.3/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.3'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build82557609=/tmp/go-build -gno-record-gcc-switches'

What did you do?

I write a demo to simulate http shutdown.
The demo start a http server and N goroutine to send http POST requests. Server and client each has a cache data, aka serverData and clientData. N goroutines are indexed by id, and eagerly send Post request to server with id carried by body. Once HTTP server receives a request, it increases the corresponding serverData and sends httpOK response. Client goroutine quits for error or increases clientData by one.
At the end of program, Server.Shutdown is called. Wait all client goroutine exit. Check whether serverData and clientData is equal.
If net/http.Server.Shutdown is perfect graceful, the following situation should not happen.

Also note that closing a connection does not terminate any associated goroutine running ServeHTTP: those goroutines will continue to run until they decide to return (perhaps as a result of noticing a failed write to the response).

Any client error caused by shutdown should be retryable. This is more friendly for client other than undefined server behavior.

An apparently fix would be checking Server.shuttingDown after readRequest but before ServeHTTP.

What did you see happen?

build and run the demo, I get the following output:

http.Server.Serve http: Server closed
Post Post "http://127.0.0.1:47681/": EOF
Post Post "http://127.0.0.1:47681/": EOF
Post Post "http://127.0.0.1:47681/": EOF
Post Post "http://127.0.0.1:47681/": EOF
Post Post "http://127.0.0.1:47681/": EOF
Post Post "http://127.0.0.1:47681/": EOF
Post Post "http://127.0.0.1:47681/": EOF
Post Post "http://127.0.0.1:47681/": EOF
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
Post Post "http://127.0.0.1:47681/": http: server closed idle connection
http.Server.Shutdown <nil>
data at index 17 mismatch, client 6091, server 6092
data at index 36 mismatch, client 6081, server 6082
data at index 59 mismatch, client 6038, server 6039
data at index 61 mismatch, client 5971, server 5972
data at index 75 mismatch, client 5979, server 5980
data at index 85 mismatch, client 6075, server 6076
data at index 99 mismatch, client 6044, server 6045

What did you expect to see?

If http.Server.Shutdown is perfect graceful, there should be no logs like data at index 99 mismatch, client 6044, server 6045.
@WineChord WineChord linked a pull request Feb 20, 2024 that will close this issue
Open
@gopherbot
Copy link

Change https://go.dev/cl/565277 mentions this issue: net/http: check server shutting down before processing the request

@thanm thanm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 20, 2024
@thanm
Copy link
Contributor

thanm commented Feb 20, 2024

@neild per owners

@odeke-em odeke-em changed the title net/http: Client errors cuased by Server.Shutdown should be always retryable. net/http: Client errors caused by Server.Shutdown should be always retryable Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

net/http: check server shutting down before processing the request WineChord/go
3 participants
@gopherbot @thanm @tocrafty