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: Request with canceled context produces undefined behaviour #43082

Open
jejefferson opened this issue Dec 8, 2020 · 1 comment
Open

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

jejefferson opened this issue Dec 8, 2020 · 1 comment

Comments

@jejefferson
Copy link

@jejefferson jejefferson commented Dec 8, 2020

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:

func (cc *http2ClientConn) roundTrip(req *Request) (res *Response, gotErrAfterReqBodyWrite bool, err error) {

Select statement don't guarantee of execution order. So with manually canceled context before request I got:

  1. first write body here (request sent):
    case err := <-bodyWriter.resc:
  2. next execution code here that returns cs.getStartedWrite() == true and error (context canceled)
    case <-ctx.Done():

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:

ctx := req.Context()

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: stripe/stripe-go#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 toothrot changed the title Request "net/http" with prepared canceled context produces undefined behaviour net/http: Request with canceled context produces undefined behaviour Dec 8, 2020
@toothrot
Copy link
Contributor

@toothrot toothrot commented Dec 8, 2020

/cc @neild

@toothrot toothrot added this to the Backlog milestone Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants