net/http: reading from hijacked bufio.Reader should not cause r.Context to be cancelled

[nhooyr]

I was working on https://nhooyr.io/websocket when I noticed that when the connection returned from Hijack is closed, r.Context() is cancelled. This is bizarre behaviour given after Hijack, there should be no way for net/http to cancel the request context. After all, how could it know the connection is closed?

I tracked down the issue to the bufio.Reader returned from Hijack. When a read errors, it causes r.Context to become cancelled.

In my library I'm using the returned bufio.Reader for all reads. When a goroutine would close the connection, another goroutine would read from the connection and so due to this line, the context would be cancelled.

I think its best that reads on the bufio.Reader not cause r.Context to be cancelled as after Hijack, net/http should not be involved at all.

[gopherbot]

Change https://golang.org/cl/179458 mentions this issue: net/http: never ever cancel r.Context() after Hijack

[odeke-em]

Thank you for reporting this issue @nhooyr!
Might you be able to perhaps work out a repro which we could turn into a test as @bradfitz has requested in your CL?

I have started a seed test case https://play.golang.org/p/gidrGk8M5Dq but just shutting down my computer to be up for a long event, please help me carry on to see it this can make your repro
(or inlined below)

package main

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

func main() {
	cst := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		hj, ok := w.(http.Hijacker)
		if !ok {
			w.WriteHeader(http.StatusNotImplemented)
			return
		}
		conn, bufw, err := hj.Hijack()
		if err != nil {
			http.Error(w, err.Error(), http.StatusInternalServerError)
			return
		}
		defer conn.Close()

		reqBlob, _ := ioutil.ReadAll(conn)
		fmt.Printf("request blob: %s\n", reqBlob)
		bufw.Write([]byte("HTTP/1.1 200 OK\r\nConnection: keep-alive\r\nContent-Encoding: chunked\r\nContent-Length: 2\r\n\r\nok"))
		bufw.Flush()
	}))
	defer cst.Close()

	ctx, cancel := context.WithCancel(context.Background())
	defer cancel()

	req, _ := http.NewRequest("POST", cst.URL, strings.NewReader("aaaaaaa*****aaaaaaaaaaaa"))
	req = req.WithContext(ctx)

	resultCh := make(chan []byte)
	go func() {
		defer close(resultCh)
		res, err := cst.Client().Do(req)
		log.Print("Made request!")
		if err != nil {
			log.Fatalf("failed http request: %v", err)
		}
		blob, _ := ioutil.ReadAll(res.Body)
		_ = res.Body.Close()
		resultCh <- blob
	}()

	select {
	case <-ctx.Done():
		log.Fatalf("Surprisingly context was ended with error: %v", ctx.Err())
	case <-time.After(5 * time.Second):
		log.Fatal("Waited for too long!")
	case blob := <-resultCh:
		// Great case!
		// fmt.Printf("%s\n", blob)
		if len(blob) == 0 {
			log.Fatal("Failed to get back response data")
		}
	}
}

[nhooyr]

The essence of the problem is replicated by Hijacking the connection, closing it, then reading from the bufio Reader returned by Hijack and observing that context.Context is cancelled.

[odeke-em]

Cool, thank you @nhooyr! So perhaps https://play.golang.org/p/k5S0LVVXFhb will suffice for your test

package main

import (
	"io/ioutil"
	"net/http"
	"net/http/httptest"
	"strings"
	"testing"
)

func TestConnCloseNoCancellation(t *testing.T) {
	cst := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		hj, ok := w.(http.Hijacker)
		defer func() {
			ctx := r.Context()
			select {
			case <-ctx.Done():
				t.Fatalf("client.Context Done with error: %v", ctx.Err())
			default:
			}
		}()
		if !ok {
			w.WriteHeader(http.StatusNotImplemented)
			return
		}
		conn, bufrw, err := hj.Hijack()
		if err != nil {
			http.Error(w, err.Error(), http.StatusInternalServerError)
			return
		}
		conn.Close()

		_, _ = ioutil.ReadAll(bufrw)
		bufrw.Write([]byte("HTTP/1.1 200 OK\r\nConnection: keep-alive\r\nContent-Encoding: chunked\r\nContent-Length: 2\r\n\r\nok"))
		bufrw.Flush()
	}))
	defer cst.Close()

	req, _ := http.NewRequest("POST", cst.URL, strings.NewReader("aaaaaaa*****aaaaaaaaaaaa"))
	res, _ := cst.Client().Do(req)
	if res != nil {
		_, _ = ioutil.ReadAll(res.Body)
		_ = res.Body.Close()
	}
}

which produces

=== RUN   TestConnCloseNoCancellation
--- FAIL: TestConnCloseNoCancellation (0.00s)
    prog.go:18: client.Context Done with error: context canceled
FAIL

[odeke-em]

Hey @nhooyr, I recall in an issue you commented that I should carry this on. @clairerhoda would like to take this on so she'll continue with it and submit it for Go1.14.

[gopherbot]

Change https://golang.org/cl/200437 mentions this issue: net/http: don't cancel hijacked connection's request context

[AlexanderYastrebov]

#27408 seems related