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

Don't pass a reused bytes.Buffer to the ES client #162

Merged
merged 2 commits into from
Apr 24, 2024
Merged

Conversation

vikmik
Copy link
Contributor

@vikmik vikmik commented Apr 24, 2024

See comments.

Directly passing a reusable bytes.Buffer is unsafe because of potential race condition on error scenarios introduced by the underlying RoundTripper implementation.

See:
https://pkg.go.dev/net/http#RoundTripper

// 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.

We don't need a full copy of the buffer underlying slice since the sending goroutine is only reading from the buffer. We just need to make sure that bytes.Buffer other member variables aren't accessed and modified concurrently by different goroutines.

The call to bytes.NewBuffer(b.buf.Bytes()) mimics what's done internally by the net/http library (and other implementations) on every retry: https://github.com/golang/go/blob/08e73e61521d7b83198407211aa232ed4f572f18/src/net/http/request.go#L929 - that is, taking the underlying byte slice and wrapping it in a brand new bytes.Buffer prior to any retry.

Note that GetBody is generally not invoked on the first try of a request, which means the original request body is directly read from. Prior to this PR, b.buf could be directly read from even after the call to req.Do returned.

Related: golang/go#51907

@elastic-apm-tech elastic-apm-tech added the safe-to-test Automated label for running bench-diff on forked PRs label Apr 24, 2024
marclop
marclop previously approved these changes Apr 24, 2024
bulk_indexer.go Outdated Show resolved Hide resolved
@vikmik vikmik marked this pull request as ready for review April 24, 2024 14:09
@vikmik vikmik requested a review from a team as a code owner April 24, 2024 14:09
@vikmik vikmik enabled auto-merge (squash) April 24, 2024 14:10
@vikmik vikmik merged commit 782e5d8 into main Apr 24, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe-to-test Automated label for running bench-diff on forked PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants