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: server.Serve() uses deprecated net.Error.Temporary() #66208

Open
geofffranks opened this issue Mar 8, 2024 · 5 comments
Open

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

geofffranks opened this issue Mar 8, 2024 · 5 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@geofffranks
Copy link

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
Copy link
Contributor

mknyszek commented Mar 9, 2024

CC @neild

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

neild commented Mar 9, 2024

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.

@seankhliao seankhliao modified the milestones: Backlog, Unplanned Mar 9, 2024
@geofffranks
Copy link
Author

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 neild mentioned this issue Mar 11, 2024
Open
@neild
Copy link
Contributor

neild commented Mar 11, 2024

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
Copy link
Author

geofffranks commented Mar 11, 2024 via email •
edited

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

No branches or pull requests
4 participants
@neild @mknyszek @geofffranks @seankhliao