search

golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.17k stars 17.56k forks source link

net/http: Request with canceled context produces undefined behaviour #43082

Open jejefferson opened 3 years ago

jejefferson commented 3 years ago

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

go version go1.15.5 darwin/amd64
OS: macOS Catalina 10.15.7

Does this issue reproduce with the latest release? Yes. Just do any request on canceled context.

What did you do?

Hi there. I got undefined behaviour of stdlib http client. The library (stripe-go) uses stdlib and relies on cancelation of request by context.Context. For some local tests I cancel passed Context manually (no timeout) calling cancel() method and then call Stripe library. I was surprised to get result together: 1) Stripe got an request and perform action 2) I got an error and will retry (thanks for idempotency key that will not cause double charging of customer)

Problem code here in function named roundTrip: https://github.com/golang/go/blob/212d385a2f723a8dd5e7d2e83efb478ddd139349/src/net/http/h2_bundle.go#L7535

Select statement don't guarantee of execution order. So with manually canceled context before request I got: 1) first write body here (request sent): https://github.com/golang/go/blob/212d385a2f723a8dd5e7d2e83efb478ddd139349/src/net/http/h2_bundle.go#L7700 2) next execution code here that returns cs.getStartedWrite() == true and error (context canceled) https://github.com/golang/go/blob/212d385a2f723a8dd5e7d2e83efb478ddd139349/src/net/http/h2_bundle.go#L7677

Could you implement some cs.completelyWritten() check and don't return error in case of body completed written? Or maybe reset Context and don't process ctx.Done for case if it happen in last nanosecond or even with already written as in my case. Or maybe even check ctx.Err() before request just for test cases here: https://github.com/golang/go/blob/212d385a2f723a8dd5e7d2e83efb478ddd139349/src/net/http/h2_bundle.go#L7638

My point is - don't return error for allow to do fewer count of retries. Unfortunately stripe-go used a retry for request with canceled so chance to send request was multiplied by retry count. It was fixed in recent: https://github.com/stripe/stripe-go/issues/1184 And yes I understand about sent request is not guarantee and we can get error while reading response. But stdlib should give guarantee with predefined environment I think. And one question more: is it documented behaviour? I will mail Stripe with this issue link . Maybe that's better to check ctx.Err() on their side before sending request with already canceled context.

What did you expect to see? Sent request or error. No both together.

What did you see instead? Server receive and process request && I got an error (context canceled) from library (not by reading response) I need in one more retry and should implement Idempotency key

toothrot commented 3 years ago

/cc @neild