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/pprof: respect timeouts set using http.ResponseController #62358

Closed
komuw opened this issue Aug 29, 2023 · 11 comments
Closed

net/http/pprof: respect timeouts set using http.ResponseController #62358

komuw opened this issue Aug 29, 2023 · 11 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.23

Comments

@komuw
Copy link
Contributor

komuw commented Aug 29, 2023

Example program with issue: https://go.dev/play/p/DoZD1wMDPhw

Most time I find myself wanting to run my http requests with tight deadlines using http.Server.WriteTimeout.
While at the same time, I would like requests to the pprof endpoints to operate under more lenient timeouts.
For example I would set http.Server.WriteTimeout to 3 seconds and at the same time want to be able to take a pprof sample that would take a long time curl -vkL "http://localhost:9192/debug/pprof/profile?seconds=30"

I thought I would use http.NewResponseController to do that, see https://go.dev/play/p/DoZD1wMDPhw
However, that fails with profile duration exceeds server's WriteTimeout.
This is because the pprof handlers only consider the server timeout;

go/src/net/http/pprof/pprof.go

Lines 117 to 120 in a9d4b2d

func durationExceedsWriteTimeout(r *http.Request, seconds float64) bool {
srv, ok := r.Context().Value(http.ServerContextKey).(*http.Server)
return ok && srv.WriteTimeout != 0 && seconds >= srv.WriteTimeout.Seconds()
}

My proposal is that they should also consider any timeout that has been set by http.ResponseController.
I don't know how such a functionality might be implemented since durationExceedsWriteTimeout takes a http.Request and its context does not contain information on whether http.ResponseController has been used.

The current workaround is to use two servers, one for application handler and one for pprof handlers each with its own http.Server.WriteTimeout. But I like having one server.
@komuw komuw added the Proposal label Aug 29, 2023
@gopherbot gopherbot added this to the Proposal milestone Aug 29, 2023
@komuw
Copy link
Contributor Author

komuw commented Aug 29, 2023

Another option would be to re-implement https://github.com/golang/go/blob/master/src/net/http/pprof/pprof.go on my own without durationExceedsWriteTimeout. However that file imports internal/profile

@ianlancetaylor
Copy link
Contributor

CC @neild @bradfitz

@komuw
Copy link
Contributor Author

komuw commented Aug 30, 2023

Another option would be to re-implement pprof.go ...
However that file imports internal/profile

Since serveDeltaProfile is the only one that uses the internal package. A way around it would be to export it.

// ServeDeltaProfile reports the difference between two measurements collected between the given seconds
// It can be used by people who want to register their own pprof handlers.
func ServeDeltaProfile(name string, w http.ResponseWriter, r *http.Request, p *pprof.Profile, secStr string)

@komuw komuw mentioned this issue Aug 30, 2023
Closed
@komuw
Copy link
Contributor Author

komuw commented Aug 30, 2023 •
edited

I found a workaround, set http.ServerContextKey to a fake server which has a long timeout; https://go.dev/play/p/KaybTdDd1ll
This solves my issue and this proposal can be closed; unless someone thinks it still has merit and wants to change net/http/pprof itself.

@AlexanderYastrebov
Copy link
Contributor

AlexanderYastrebov commented Aug 30, 2023 •
edited

I guess it could also be an exported function:

// SetWriteTimeout configures write timeout for  HTTP handlers
func SetWriteTimeout(timeout time.Duration)

@komuw komuw mentioned this issue Sep 1, 2023
Open
@lukestoward
Copy link

This solves my issue and this proposal can be closed; unless someone thinks it still has merit and wants to change net/http/pprof itself.

I have just stumbled on to this issue myself. I believe there is still merit in making the net/http/pprof more aware of timeout controls.

AlexanderYastrebov added a commit to AlexanderYastrebov/go that referenced this issue Nov 23, 2023
@AlexanderYastrebov
net/http/pprof: configure WriteDeadline
b79ae38 
Configure write deadline according to requested profiling duration.

Fixes golang#62358
@AlexanderYastrebov AlexanderYastrebov mentioned this issue Nov 23, 2023
Closed
@gopherbot
Copy link

Change https://go.dev/cl/544756 mentions this issue: net/http/pprof: configure WriteDeadline

@AlexanderYastrebov
Copy link
Contributor

I think we can adjust write timeout based on the requested profiling duration, see https://go-review.googlesource.com/c/go/+/544756

@neild
Copy link
Contributor

neild commented Nov 27, 2023

Is there any good reason for net/http/pprof not to disable the write timeout entirely on its handlers?

@rsc
Copy link
Contributor

rsc commented Nov 29, 2023

Looked at the CL. I don't believe this needs a proposal but thanks for checking with us.

@rsc rsc changed the title proposal: net/http/pprof: respect timeouts set using http.ResponseController net/http/pprof: respect timeouts set using http.ResponseController Nov 29, 2023
@rsc rsc removed the Proposal label Nov 29, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Nov 29, 2023
@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 29, 2023
@dmitshur dmitshur modified the milestones: Proposal, Backlog Nov 29, 2023
@seankhliao seankhliao mentioned this issue Dec 2, 2023
Closed
@AlexanderYastrebov
Copy link
Contributor

I think the fix will benefit PGO.
@mknyszek What's the processes to merge "Ready to submit" CL?

@dmitshur dmitshur modified the milestones: Backlog, Go1.23 Jan 9, 2024
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
@AlexanderYastrebov @ezz-no
net/http/pprof: configure WriteDeadline
afd9dd4 
Configure write deadline according to requested profiling duration.

Fixes golang#62358

Change-Id: I2350110ff20a637c7e90bdda57026b0b0d9c87ba
GitHub-Last-Rev: b79ae38
GitHub-Pull-Request: golang#64360
Reviewed-on: https://go-review.googlesource.com/c/go/+/544756
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.23
Development

Successfully merging a pull request may close this issue.

net/http/pprof: configure WriteDeadline AlexanderYastrebov/go
8 participants
@neild @rsc @AlexanderYastrebov @dmitshur @ianlancetaylor @komuw @lukestoward @gopherbot