http3: return the context cancellation error from RoundTrip

[marten-seemann]

@jhump Can you try out this PR? Does it fix your problem?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0489409) 83.71% compared to head (7c2ca28) 83.74%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4203      +/-   ##
==========================================
+ Coverage   83.71%   83.74%   +0.03%     
==========================================
  Files         150      150              
  Lines       15470    15477       +7     
==========================================
+ Hits        12950    12960      +10     
+ Misses       2021     2019       -2     
+ Partials      499      498       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jhump]

@marten-seemann, on the face of it, this looks right, but I'm seeing something weird in my client program that I need to track down. One thing that is absent here is doing something similar for errors returned from Body.Read, since consuming the response data can also encounter context timeout/cancellation.

[jhump]

Another useful test would be to verify receipt of deadline exceeded, which looks very similar to this case.

	It("handles context deadline exceeded", func() {
		mux.HandleFunc("/cancel", func(w http.ResponseWriter, r *http.Request) {
			<-r.Context().Done()
		})

		ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond)
		req, err := http.NewRequestWithContext(ctx, http.MethodGet, fmt.Sprintf("https://localhost:%d/cancel", port), nil)
		Expect(err).ToNot(HaveOccurred())

		_, err = client.Do(req)
		Expect(err).To(HaveOccurred())
		Expect(err).To(MatchError(context.DeadlineExceeded))
	})

[jhump]

A test case for the body-read error wouldn't be much different:

	It("handles context cancellations when reading body", func() {
		mux.HandleFunc("/cancel", func(w http.ResponseWriter, r *http.Request) {
			w.WriteHeader(http.StatusOK)
			<-r.Context().Done()
		})

		ctx, cancel := context.WithCancel(context.Background())
		req, err := http.NewRequestWithContext(ctx, http.MethodGet, fmt.Sprintf("https://localhost:%d/cancel", port), nil)
		Expect(err).ToNot(HaveOccurred())
		time.AfterFunc(50*time.Millisecond, cancel)

		resp, err = client.Do(req)
		Expect(err).ToNot(HaveOccurred())
		_, err = resp.Body.Read(make([]byte, 1024))
		Expect(err).To(HaveOccurred())
		Expect(err).To(MatchError(context.Canceled))
	})

[marten-seemann]

A test case for the body-read error wouldn't be much different:

That's a much bigger change. The Go documentation for the request context says:

For an outgoing client request, the context controls the entire lifetime of a request and its response: obtaining a connection, sending the request, and reading the response headers and body.

So you're right, the body needs to return the context error as well. Currently, we don't even pass the context to the body, and it's not clear to me how to correctly respect the context without spawning a new Go routine, since the Stream.Read call is blocking call.

[marten-seemann]

I suggest doing this in a follow-up PR. I'll open a new issue.

[marten-seemann]

Merging this PR, since it at least partially fixes #4196. The remainder of the fix is tracked in #4205.