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
Don't use sync.Pool for json.NewEncoder target buffers #421
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pborzenkov do you mean usage of
sync.Pool
withjson.NewEncoder
has an issue at language level?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeevatkm No. What I mean is that
bytes.Buffer
is returned back tosync.Pool
beforenoescapeJSONMarshal
returns (due todefer
). Yet the underlying byte slice is used later, asbytes.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 byjson.NewEncoder
is actually used.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pborzenkov Thanks. My understanding is
defer
executes after thereturn
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeevatkm
Yes, it will execute after the
return
. The problem is thatreturn buf.Bytes()
does not produce a copy of the data, it return the underlying slice. And since thebuf
is returned back to the pool it can easily get reused.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeevatkm
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 theexecute
method, there isdefer
that releases request body back to the pool:But since on the first entry to this function (e.g. not a request retry) the
bodyBuf
isnil
, the call toreleaseBuffer
is basically a no-op. The trivial fix would be to do it like this:But this is probably racy, as
RoundTripper
may hold the body even afterhttp.Client.Do
returns: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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeevatkm Yeah. Gonna try and fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.