Don't use sync.Pool for json.NewEncoder target buffers #421
Conversation
Codecov Report
@@ Coverage Diff @@
## master #421 +/- ##
==========================================
- Coverage 95.97% 95.82% -0.15%
==========================================
Files 10 10
Lines 1019 1055 +36
==========================================
+ Hits 978 1011 +33
- Misses 23 25 +2
- Partials 18 19 +1
Continue to review full report at Codecov.
|
| @@ -839,9 +839,8 @@ func (r *Request) initValuesMap() { | |||
| } | |||
|
|
|||
| var noescapeJSONMarshal = func(v interface{}) ([]byte, error) { | |||
| buf := acquireBuffer() | |||
There was a problem hiding this comment.
@pborzenkov do you mean usage of sync.Pool with json.NewEncoder has an issue at language level?
There was a problem hiding this comment.
@jeevatkm No. What I mean is that bytes.Buffer is returned back to sync.Pool before noescapeJSONMarshal returns (due to defer). Yet the underlying byte slice is used later, as bytes.Buffer.Bytes() doesn't copy the data.
So a concurrently running goroutine can get the very same bytes.Buffer from the pool and overwrite its contents before the data produced by json.NewEncoder is actually used.
There was a problem hiding this comment.
@pborzenkov Thanks. My understanding is defer executes after the return statement executed. Maybe I have to read, try it out, and then get back to you.
https://golang.org/ref/spec#Defer_statements
There was a problem hiding this comment.
Yes, it will execute after the return. The problem is that return buf.Bytes() does not produce a copy of the data, it return the underlying slice. And since the buf is returned back to the pool it can easily get reused.
There was a problem hiding this comment.
@pborzenkov I think I missed this part "doesn't copy the data" from your previous response. I want to keep the sync.Pool usage instead of creating new. It's better to restructure the method flow to make use of the pool properly.
Thanks for bringing it up 👍
There was a problem hiding this comment.
Yeah, that's probably a good idea. Thanks!
BTW, there is another problem with how sync.Pool is used. When a request is made by the execute method, there is defer that releases request body back to the pool:
// Executes method executes the given `Request` object and returns response
// error.
func (c *Client) execute(req *Request) (*Response, error) {
defer releaseBuffer(req.bodyBuf)
// Apply Request middleware
But since on the first entry to this function (e.g. not a request retry) the bodyBuf is nil, the call to releaseBuffer is basically a no-op. The trivial fix would be to do it like this:
// Executes method executes the given `Request` object and returns response
// error.
func (c *Client) execute(req *Request) (*Response, error) {
defer func() {
releaseBuffer(req.bodyBuf)
}()
// Apply Request middleware
But this is probably racy, as RoundTripper may hold the body even after http.Client.Do returns:
// RoundTrip should not modify the request, except for // consuming and closing the Request's Body. RoundTrip may // read fields of the request in a separate goroutine. Callers // should not mutate or reuse the request until the Response's // Body has been closed. // // RoundTrip must always close the body, including on errors, // but depending on the implementation may do so in a separate // goroutine even after RoundTrip returns. This means that // callers wanting to reuse the body for subsequent requests // must arrange to wait for the Close call before doing so.
So the correct fix is to wrap Response.Body and release the request buffer whenever the response body is closed.
There was a problem hiding this comment.
@pborzenkov Your explanation and details make sense. Do you want to take a shot at it in this PR? or I will try it when I get a chance in the upcoming days.
There was a problem hiding this comment.
@jeevatkm Yeah. Gonna try and fix it.
There was a problem hiding this comment.
@pborzenkov I'm sorry for the delayed response at my end. Just went through your updates, it looks good. Also thanks for doing other improvements along with sync pool enhancement.
The slice returned by `noescapeJSONMarshal` continues to be accessed even after `pool.Put` was called on the buffer. This might lead to incorrect data being sent in request body if the buffer is acquired and re-written by a concurrently running goroutine. So make sure we release the buffer once the request completes.
net/http.RoundTripper may access request body in a separate goroutine, so we need to wait release the buf back to sync.Pool only after the body is closed. From the docs: // RoundTrip must always close the body, including on errors, // but depending on the implementation may do so in a separate // goroutine even after RoundTrip returns. This means that // callers wanting to reuse the body for subsequent requests // must arrange to wait for the Close call before doing so.
The slice returned by
noescapeJSONMarshalcontinues to be accessedeven after
pool.Putwas called on the buffer. This might lead toincorrect data being sent in request body if the buffer is acquired and
re-written by a concurrently running goroutine.