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.

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

net/http: document TimeoutHandler defaults #22821

Open
SamWhited opened this Issue Nov 20, 2017 · 5 comments

Comments

Projects
None yet
6 participants
@SamWhited
Member

SamWhited commented Nov 20, 2017

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

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

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

@meirf

This comment has been minimized.

Member

meirf commented May 12, 2018

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:

  1. Add to the API.
  2. 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

This comment has been minimized.

Member

agnivade commented Jul 11, 2018

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

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jul 25, 2018

@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.

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

@gopherbot

This comment has been minimized.

gopherbot commented Jul 25, 2018

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

@SamWhited

This comment has been minimized.

Member

SamWhited commented Jul 25, 2018

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.

gopherbot pushed a commit that referenced this issue Jul 31, 2018

net/http: expand a TimeoutHandler test a bit
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>

jeet-parekh added a commit to jeet-parekh/go that referenced this issue Jul 31, 2018

net/http: expand a TimeoutHandler test a bit
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment