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

Jump to bottom

net/http: body argument to NewRequest is closed multiple times #19186

Closed
erikdubbelboer opened this issue Feb 19, 2017 · 12 comments
Closed

net/http: body argument to NewRequest is closed multiple times #19186

erikdubbelboer opened this issue Feb 19, 2017 · 12 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.9Maybe

Comments

@erikdubbelboer
Copy link
Contributor

When passing an ReadCloser as body to http.NewRequest, it is possible the Close() of the ReadCloser is called multiple times for the same request. I'm wondering if this is allowed to happen, in which case it should be documented. Or if this is a bug?

it only seems to happen in rare cases where the full body is send (first close) but there is an error reading the response (second close).

Here are the stack traces which both called Close() on the same ReadCloser:

goroutine 1475 [running]:
runtime/debug.Stack(0x18, 0xc420d17ce0, 0x0)
        /usr/local/go/src/runtime/debug/stack.go:24 +0x79
github.com/mailru/easyjson/buffer.(*readCloser).Close(0xc4244fe140, 0xb2b600, 0x7fb44aacbf80)
        /home/ubuntu/src/github.com/mailru/easyjson/buffer/pool.go:263 +0x211
net/http.(*transferWriter).WriteBody(0xc424502180, 0xad2c40, 0xc422ca3480, 0x2, 0x2)
        /usr/local/go/src/net/http/transfer.go:332 +0x427
net/http.(*Request).write(0xc4244f8d00, 0xad2c40, 0xc422ca3480, 0x100000000410600, 0x0, 0x0, 0x0, 0x0)
        /usr/local/go/src/net/http/request.go:622 +0x6e9
net/http.(*persistConn).writeLoop(0xc422cd6240)
        /usr/local/go/src/net/http/transport.go:1707 +0x1ad
created by net/http.(*Transport).dialConn
        /usr/local/go/src/net/http/transport.go:1118 +0xa5a


goroutine 3775 [running]:
runtime/debug.Stack(0x0, 0xc424500c00, 0x304)
        /usr/local/go/src/runtime/debug/stack.go:24 +0x79
github.com/mailru/easyjson/buffer.(*readCloser).Close(0xc4244fe140, 0xc4244f8d00, 0xc421a7acc8)
        /home/ubuntu/src/github.com/mailru/easyjson/buffer/pool.go:261 +0xeb
net/http.(*Request).closeBody(0xc4244f8d00)
        /usr/local/go/src/net/http/request.go:1295 +0x43
net/http.(*Client).Do.func1(0xad2fc0, 0xc420011560, 0x0, 0x0)
        /usr/local/go/src/net/http/client.go:507 +0x59
net/http.(*Client).Do(0xc420106060, 0xc4244f8d00, 0x5f5e100, 0xc4244dd920, 0x7fb446aac5a0)
        /usr/local/go/src/net/http/client.go:602 +0x3d2
github.com/atomx/core/httpq.Do.func1(0xc4244f8d00, 0x5f5e100, 0xc4244dd860)
        /home/ubuntu/src/github.com/atomx/core/httpq/http.go:51 +0xe2
created by github.com/atomx/core/httpq.Do
        /home/ubuntu/src/github.com/atomx/core/httpq/http.go:56 +0x71
erikdubbelboer added a commit to erikdubbelboer/easyjson that referenced this issue Feb 19, 2017
@erikdubbelboer
Prevent bugs caused by multiple Close
e187266 
See: golang/go#19186
When the body isn't fully read and Close is called multiple times we put
the same buffer in the pool multiple times causing it to be reused by
multiple goroutines at the same time.
@erikdubbelboer erikdubbelboer mentioned this issue Feb 19, 2017
Merged
@bradfitz
Copy link
Contributor

Yeah, I feel like I've seen this too, or that there was an existing comment on a CL or a bug or TODO about this. Thanks for filing.

@bradfitz bradfitz added this to the Go1.9Maybe milestone Feb 19, 2017
@bradfitz bradfitz added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Feb 19, 2017
@nerdatmath
Copy link
Contributor

I'm taking a look at this.

@nerdatmath
Copy link
Contributor

I have a test that replicates the double close.

@gopherbot
Copy link

CL https://golang.org/cl/37298 mentions this issue.

@nerdatmath
Copy link
Contributor

I have a fix. Check it out and let me know what you think.

@odeke-em
Copy link
Member

@nerdatmath please check the CL, I think one more round to fix some docs then we'll be gucci.

@nerdatmath
Copy link
Contributor

nerdatmath commented Jun 25, 2017 via email

@bradfitz
Copy link
Contributor

@nerdatmath, no, 9 days is too late. But somebody can take it over before then. Ideally in the next 24 hours. @odeke-em?

@nerdatmath
Copy link
Contributor

Working on it right now...

@davecheney
Copy link
Contributor

Why not permit it? What is the harm in allowing close to be called multiple times, the error is ignored anyway.

@davecheney davecheney reopened this Jun 25, 2017
@bradfitz
Copy link
Contributor

Why not permit it? What is the harm in allowing close to be called multiple times, the error is ignored anyway.

Because the io.Closer interface does not specify how implementations must behave on double Close. In fact, it says:

https://golang.org/pkg/io/#Closer

The behavior of Close after the first call is undefined.

So the conservative thing to do as a user of an io.Closer is not ever call it twice.

The second call might panic or deadlock.

@davecheney
Copy link
Contributor

davecheney commented Jun 26, 2017 via email

@golang golang locked and limited conversation to collaborators Jun 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.9Maybe
Development

No branches or pull requests
6 participants
@bradfitz @davecheney @erikdubbelboer @nerdatmath @odeke-em @gopherbot