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.

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: clarify that (*Request).WithContext affects the Response too #26101

Closed
mark-rushakoff opened this issue Jun 28, 2018 · 6 comments
Closed

Comments

@mark-rushakoff
Copy link
Contributor

What version of Go are you using (go version)?

https://golang.org/pkg/net/http/#Request.Context

Does this issue reproduce with the latest release?

Same docs on https://tip.golang.org/pkg/net/http/#Request.Context

What did you do?

I read:

For outgoing client requests, the context controls cancelation.

I thought the context only affected its associated Request, and that the Response returned from Client.Do(Request) would be unaffected by the context.

I searched the net/http Godoc for any mentions of Context relating to a Response but didn't find anything.

Finally, after a long discussion with coworkers, we came up with a manual test case demonstrating that the request context does in fact affect the response:

package main

import (
	"context"
	"io"
	"io/ioutil"
	"log"
	"net/http"
	"net/http/httptest"
	"time"
)

func main() {
	s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		log.Println("serving", r.URL)
		io.WriteString(w, "Hold on...")
		if f, ok := w.(http.Flusher); ok {
			f.Flush()
		} else {
			log.Println("cannot flush!")
		}
		time.Sleep(10 * time.Second)
		io.WriteString(w, "... okay")
	}))
	defer s.Close()

	req, err := http.NewRequest("GET", s.URL, nil)
	if err != nil {
		log.Fatal("Error making request:", err)
	}

	ctx, cancel := context.WithTimeout(context.Background(), time.Second)
	defer cancel()

	req = req.WithContext(ctx)

	resp, err := http.DefaultClient.Do(req)
	if err != nil {
		panic(err)
	}
	defer resp.Body.Close()

	log.Println("client made request, starting to read response (this should time out in 1s)")
	body, err := ioutil.ReadAll(resp.Body)
	if err != nil {
		log.Fatal("Error reading response body:", err)
	}
	log.Println("client read response:", string(body))
}

// Output like:
// 2018/06/27 17:07:06 serving /
// 2018/06/27 17:07:06 client made request, starting to read response (this should time out in 1s)
// 2018/06/27 17:07:07 Error reading response body:context deadline exceeded

We would have saved a lot of time today if the doc said something more like "For outgoing client requests and their corresponding responses, the context controls cancelation."

@agnivade agnivade added this to the Unplanned milestone Jun 28, 2018
@meirf
Copy link
Contributor

meirf commented Jun 28, 2018

It might be worthwhile to be more explicit on the details before deciding on the best documentation to balance conciseness and accuracy.
Expect: The request's context has dominion for the lifetime of http.DefaultClient.Do(req). As soon as Do returns/the initial headers are received, the context no longer applies.
Actual: If the response to a request has a body, the request's context has dominion until the response body is closed. (Notable exclusion being HEADs.)
Is this correct? Would you like to upload your suggestion?

Somewhat related: #23262

@mark-rushakoff
Copy link
Contributor Author

Is your Actual note how it actually behaves?

I'm not sure of the precise actual behavior, and I'm also unsure of the correct language to use to be brief and accurate regarding the relationship between a context, request, and response; so I'm going to leave it to the Go authors or a more-ambitious-than-me contributor to take on a CL for this doc update.

@nhooyr
Copy link
Contributor

nhooyr commented Aug 13, 2018

Actual: If the response to a request has a body, the request's context has dominion until the response body is closed. (Notable exclusion being HEADs.)

That sounds fine to me to be the new documentation. Re the exclusions, the exclusion is any time there is no body or expected body correct?

@mark-rushakoff
Copy link
Contributor Author

@bradfitz care to weigh in on the docs change being discussed here?

@bradfitz bradfitz self-assigned this Aug 13, 2018
@bradfitz
Copy link
Contributor

Sent https://go-review.googlesource.com/c/go/+/129155

@gopherbot
Copy link

Change https://golang.org/cl/129155 mentions this issue: net/http: update request cancelation docs

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants