net/http: set response headers on TimeoutHandler timeout #22821

SamWhited · 2017-11-20T19:48:30Z

I was recently suprised to discover that http.TimeoutHandler returns a blob of HTML ("<html><head><title>Timeout</title></head><body><h1>Timeout</h1></body></html>") as the default message body. It does not attempt to set the appropriate Content-Type header and if you write a response yourself there is no option to set the content type.

The documentation says:

(If msg is empty, a suitable default message will be sent.)

It would be nice if the format of the default message could be mentioned in the documentation, the default could be removed, or the Content-Type could be set (or some combination of the above).

/cc @bradfitz

The text was updated successfully, but these errors were encountered:

meirf · 2018-05-12T19:51:39Z

Of the recommendations, IMO the only thing to do is mention what the default message is. (And for Go2, use a different API that let's you specify content type.)

In terms of setting Content-Type, there seem to be two options:

Add to the API.
Do not add to the API - detect content type.

If we choose 1, the new function could be something like

// ... if body or contentType is empty, "<html><head><title>Timeout</title></head><body>
// <h1>Timeout</h1></body></html>" and "text/html; charset=utf-8" will be used
TimeoutHandlerCustomContentType(h Handler, dt time.Duration, body []byte, contentType string) Handler

If we choose 2, we could DetectContentType. (For performance, this would happen once - when http.TimeoutHandler is called.) IMO neither of these options are good. Option 1 doesn't seem to provide enough value to warrant cluttering/adding to the API. Option 2 doesn't seem great since DetectContentType is not so full featured. (@SamWhited do you remember the type of content type you had in mind?)

Assuming the API is not added to, removing the default message could be bad user experience - browser returning a 503 without any body might be considered by users as loss of internet connectivity, depending on how clear the browser explains the problem. (It might also be considered backwards incompatible.)

agnivade · 2018-07-11T04:39:45Z

@bradfitz - this is going in the 1.11 train ? We just need a decision here regarding what is to be done.

bradfitz · 2018-07-25T18:07:55Z

@meirf, it already uses DetectContentType implicitly. I just verified. The Content-Type it's sending now is text/html; charset=utf-8. If I change the default error message to "boom", the Content-Type becomes text/plain; charset=utf-8.

@SamWhited, can you elaborate what the problem was?

I'm going to kick this to Unplanned until there's more detail.

gopherbot · 2018-07-25T18:08:50Z

Change https://golang.org/cl/125855 mentions this issue: net/http: expand a TimeoutHandler test a bit

SamWhited · 2018-07-25T19:14:49Z

do you remember the type of content type you had in mind?

Unfortunately the "real world" code I originally ran into this problem with no longer exists and timeout handler isn't being used in the codebase anymore, however it was part of the REST API, I think, so I'm assuming it was JSON.

it already uses DetectContentType implicitly. I just verified.

Am I doing something foolish then? This suprised me so I threw together this test to check and it is failing as expected. Even if the DetectContentType call were happening at some higher level in the stack, this was causing problems with prod for me so maybe wherever it happens isn't always triggered (I haven't looked yet to see where this might happen):

var contentTypeTests = [...]struct {
	body        string
	contentType string
}{
	0: {
		body:        "",
		contentType: "text/html; charset=utf-8",
	},
	1: {
		body:        "test",
		contentType: "text/plain; charset=utf-8",
	},
}

func TestTimeoutHandler(t *testing.T) {
	for i, tc := range contentTypeTests {
		t.Run(strconv.Itoa(i), func(t *testing.T) {
			h := TimeoutHandler(HandlerFunc(func(w ResponseWriter, r *Request) {
				time.Sleep(2 * time.Second)
			}), time.Second, tc.body)
			w := httptest.NewRecorder()
			h.ServeHTTP(w, httptest.NewRequest("GET", "/", nil))
			const defBody = `<html><head><title>Timeout</title></head><body><h1>Timeout</h1></body></html>`
			if s := w.Body.String(); (tc.body == "" && s != defBody) || (tc.body != "" && s != tc.body) {
				t.Errorf("Bad body: want=%q, got=%q", tc.body, s)
			}
			if s := w.HeaderMap.Get("Content-Type"); s != tc.contentType {
				t.Errorf("Bad CT: want=%q, got=%q", tc.contentType, s)
			}
		})
	}
}

can you elaborate what the problem was?

The handler was replying with a 503, but no content-type header was being set which was expected by a client using the service. I couldn't wrap the handler in something else because WriteHeader had already been called, so there was no good way for me to set a content-type if the timeouthandler fires.

Updates #22821 Change-Id: I2d0d483538174a90f56c26d99bea89fe9ce4d144 Reviewed-on: https://go-review.googlesource.com/125855 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Andrew Bonventre <andybons@golang.org>

Updates golang#22821 Change-Id: I2d0d483538174a90f56c26d99bea89fe9ce4d144 Reviewed-on: https://go-review.googlesource.com/125855 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Andrew Bonventre <andybons@golang.org>

bentcoder · 2021-06-12T21:55:04Z

While this has been hanging for a while and don't know how long more it will hang, what can we do with JSON in the meanwhile? The algorithm doesn't seem to work for JSON if I don't misunderstand. See my example please: #46721

bentcoder · 2021-08-22T09:00:00Z

Is there a plan of action about this request?

AlexanderYastrebov · 2021-10-21T14:28:46Z

#46721 was marked as a duplicate of this while IMO it was more a feature request: allow timeout response customization.
The generic way would be to extend timeout handler with an error handler that would take care of status, headers and response body on timeout.

neild · 2021-10-21T23:25:34Z

(Retitling this issue, since I think the real desire is a way to set Content-Type on timeout.)

If we want to give the user more control over the response when a timeout occurs, then I think the right thing to is for the user to provide two Handlers. Something like this, maybe:

// TimeoutHandler2 returns a Handler that runs h with the given time limit.
//
// The new Handler calls h.ServeHTTP to handle each request, but if a
// call runs for longer than its time limit, the handler calls timeoutHandler.ServeHTTP.
// After such a timeout, writes by h to its ResponseWriter will return ErrHandlerTimeout.
func TimeoutHandler2(h, timeoutHandler Handler, dt time.Duration) Handler {
}

I don't know what we'd call TimeoutHandler2.

Another possibility would be to call h.ServeHTTPTimeout when present; that'd be a bit clunkier to use, but avoids adding a new function.

AlexanderYastrebov · 2021-10-22T15:07:44Z

ReverseProxy has ErrorHandler with similar purpose introduced within #22700 by https://go-review.googlesource.com/c/go/+/77410/

Fixes golang#22821 Fixes golang#48948

AlexanderYastrebov · 2021-10-22T17:50:51Z

I guess either a new constructor #22821 (comment) or a new interface would be required, e.g.:

type HandlerWithTimeout interface {
	Handler
	// ServeTimeout writes timeout error response.
	// Use r.Context().Err() to obtain the error value.
	ServeTimeout(w ResponseWriter, r *Request)
}

see #49119

gopherbot · 2021-10-22T17:53:16Z

Change https://golang.org/cl/357918 mentions this issue: net/http: enable TimeoutHandler custom error response

Fixes golang#22821 Fixes golang#48948

gopherbot added the Documentation label Nov 20, 2017

odeke-em changed the title ~~Document http.TimeoutHandler defaults~~ net/http: document TimeoutHandler defaults Nov 20, 2017

bradfitz added this to the Go1.11 milestone Nov 20, 2017

ALTree added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 16, 2018

bradfitz modified the milestones: Go1.11, Unplanned Jul 25, 2018

bradfitz added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jul 25, 2018

seankhliao mentioned this issue Jun 12, 2021

net/http: TimeoutHandler with header argument #46721

Closed

ALTree removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jun 18, 2021

neild changed the title ~~net/http: document TimeoutHandler defaults~~ net/http: set response headers on TimeoutHandler timeout Oct 21, 2021

neild added FeatureRequest and removed Documentation labels Oct 21, 2021

AlexanderYastrebov mentioned this issue Oct 22, 2021

net/http: TimeoutHandler should distinguish between timeout and requests closed by clients. #48948

Closed

AlexanderYastrebov added a commit to AlexanderYastrebov/go that referenced this issue Oct 22, 2021

net/http: enable TimeoutHandler custom error response

7600c82

Fixes golang#22821 Fixes golang#48948

AlexanderYastrebov mentioned this issue Oct 22, 2021

net/http: enable TimeoutHandler custom error response #49119

Closed

AlexanderYastrebov added a commit to AlexanderYastrebov/go that referenced this issue Oct 22, 2021

net/http: enable TimeoutHandler custom error response

f7877b5

Fixes golang#22821 Fixes golang#48948

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net/http: set response headers on TimeoutHandler timeout #22821

net/http: set response headers on TimeoutHandler timeout #22821

SamWhited commented Nov 20, 2017 •

edited

meirf commented May 12, 2018 •

edited

agnivade commented Jul 11, 2018

bradfitz commented Jul 25, 2018

gopherbot commented Jul 25, 2018

SamWhited commented Jul 25, 2018 •

edited

bentcoder commented Jun 12, 2021

bentcoder commented Aug 22, 2021

AlexanderYastrebov commented Oct 21, 2021

neild commented Oct 21, 2021 •

edited

AlexanderYastrebov commented Oct 22, 2021

AlexanderYastrebov commented Oct 22, 2021

gopherbot commented Oct 22, 2021

net/http: set response headers on TimeoutHandler timeout #22821

net/http: set response headers on TimeoutHandler timeout #22821

Comments

SamWhited commented Nov 20, 2017 • edited

meirf commented May 12, 2018 • edited

agnivade commented Jul 11, 2018

bradfitz commented Jul 25, 2018

gopherbot commented Jul 25, 2018

SamWhited commented Jul 25, 2018 • edited

bentcoder commented Jun 12, 2021

bentcoder commented Aug 22, 2021

AlexanderYastrebov commented Oct 21, 2021

neild commented Oct 21, 2021 • edited

AlexanderYastrebov commented Oct 22, 2021

AlexanderYastrebov commented Oct 22, 2021

gopherbot commented Oct 22, 2021

SamWhited commented Nov 20, 2017 •

edited

meirf commented May 12, 2018 •

edited

SamWhited commented Jul 25, 2018 •

edited

neild commented Oct 21, 2021 •

edited