net/http: server.Serve() uses deprecated net.Error.Temporary()

[geofffranks]

Go version

1.22

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/pivotal/.cache/go-build'
GOENV='/home/pivotal/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/pivotal/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/pivotal/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.22.0'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/pivotal/workspace/diego-release/src/code.cloudfoundry.org/go.mod'
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-build3180483851=/tmp/go-build -gno-record-gcc-switches'

What did you do?

staticcheck -tests=false -checks SA1019 net/http | grep- i temporary

What did you see happen?

/usr/local/go/src/net/http/server.go:3260:40: ne.Temporary has been deprecated since Go 1.18 because it shouldn't be used: Temporary errors are not well-defined. Most "temporary" errors are timeouts, and the few exceptions are surprising. Do not use this method. (SA1019)

What did you expect to see?

While trying to investigate the proper way to update some of our codebases that relied on the deprecated net.Error.Temporary() function, I went to net/http.Server.Serve example of what to do best. Unfortunately, the logic here is also using the deprecated Temporary() function. It also is using an up to 1s backoff delay before accepting the next connection. This seems undesirable for performance, or am I misunderstanding what the code is doing here?

[mknyszek]

CC @neild

[neild]

This is, I believe, mostly checking for EMFILE or ENFILE errors, indicating that the process or system has run out of file descriptors. The backoff is based on the presumption that if we wait and try again, FDs may have become available. Under normal operations, these errors do not occur and no backoff is performed.

Apparently ECONNRESET and ECONNABORTED as well. I'm not sure under what circumstances those might occur.

We could try to modernize the http.Server code, but it appears to work now. If we shuffle things around so that we don't call Temporary but have the exact same behavior as today, that's churn without a lot of benefit. If we make functional changes, we'd first need to understand what those should be.

[geofffranks]

If we make functional changes, we'd first need to understand what those should be.

I agree. However, this feels like having ones cake and eating it too. If such a fundamental part of Golang as the http.Server continues to use it, either net.Error.Temporary() shouldn't be deprecated, or there should be clearer documentation of what errors that function would return true for, so that http.Server and other codebases using it can properly account for changes to stop using it.

[neild]

net.Error.Temporary is deprecated because it was overly broad and demonstrably confusing. The fact that you can use it safely in one very limited circumstance doesn't change that.

However, we should have a non-deprecated replacement for the case of testing for transient Accept errors. Filed #66252 with a concrete proposal.

[geofffranks]

Perfect, thank you