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

x/net/context/ctxhttp: Request not cancelled once .Do() returns #13325

Closed
pwaller opened this issue Nov 19, 2015 · 3 comments
Closed

x/net/context/ctxhttp: Request not cancelled once .Do() returns #13325

pwaller opened this issue Nov 19, 2015 · 3 comments

Comments

@pwaller
Copy link
Contributor

pwaller commented Nov 19, 2015

If a HTTP request is cancelled via a context once .Do() has returned, the cancellation request is ignored.

Consider this test in x/net/context/ctxhttp:

func TestCancelAfterRequest(t *testing.T) {
    ctx, cancel := context.WithCancel(context.Background())

    resp, err := doRequest(ctx)

    // Cancel before reading the body.
    // Request.Body should still be readable after the context is canceled.
    cancel()

    b, err := ioutil.ReadAll(resp.Body)
    if err != nil || string(b) != requestBody {
        t.Fatalf("could not read body: %q %v", b, err)
    }
}

Unfortunately, I think the test could be problematic for many useful cases for cancelling http requests.

The immediate use case I have is to attach to the stdout of a docker container over docker's http endpoint, and disconnect when a client's interest in the output goes away. Otherwise resources are leaked, since the stdout might not otherwise go away. See fsouza/go-dockerclient#161. Secondly, cancelling a build once it is in progress, where the build output is being streamed to you: fsouza/go-dockerclient#182.

Here's an alternative "test" which simulates what I described above, exercising cancellation once Do() has returned. Currently, it hangs forever:

func TestCancelAfterRequest(t *testing.T) {
    var okHandler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        w.WriteHeader(http.StatusOK)
        <-w.(http.CloseNotifier).CloseNotify()
    })

    serv := httptest.NewServer(okHandler)
    defer serv.Close()

    ctx, cancel := context.WithCancel(context.Background())
    resp, err := Get(ctx, nil, serv.URL)

    cancel()

    var bs [1]byte
    n, err := resp.Body.Read(bs[:])
    log.Printf("n, err := %v, %v", n, err)
}

It's fairly obvious from reading the implementation that the canceler has no effect called once .Do() returns, however it is not obvious from the documentation.

See also #13293, and golang-dev discussion.

@okdave
Copy link
Contributor

okdave commented Jan 4, 2016

I'm taking a look at this now. I double checked that closing the request's Cancel channel after Do return does behave like you'd expect, so I'm just trying to work out the best way to plumb that into the ctxhttp package.

FWIW, there is a slight bug in the test above: since the headers are never being flushed the call to Do is never even returning. You can fix that by writing sufficient data to the body, or by calling Flush:

 w.WriteHeader(http.StatusOK)
 w.(http.Flusher).Flush()

@okdave okdave self-assigned this Jan 4, 2016
@pwaller
Copy link
Contributor Author

pwaller commented Jan 26, 2016

@okdave was this supposed to be closed when https://go-review.googlesource.com/#/c/18188/ was merged?

@bradfitz
Copy link
Contributor

Looks like it.

@golang golang locked and limited conversation to collaborators Feb 3, 2017
@rsc rsc unassigned okdave Jun 23, 2022
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

5 participants