Closed jsha closed 2 years ago
@jsha, thank you for the issue and details! We saw that SIGSEGV in some of our internal Go services when we bumped to 1.17.5. This is on the queue to investigate further in libhoney.
I have a fix, I'll put a PR up.
PR is up.
Closed by #156
We're going to smoketest this within Honeycomb a few days; if you need the fix sooner, please run:
go mod edit -replace github.com/honeycombio/libhoney-go=github.com/honeycombio/libhoney-go@main
go mod tidy
Thanks for the report!
In letsencrypt/boulder, we deployed a build using Go 1.17.5 (upgrading from 1.17). Immediately we started getting panics from our web frontends like this:
libhoney is the only component in our frontends that makes outbound requests with
net/http
(we also use gRPC, but it has an internal h2 library).When we downgraded to Go 1.17, the panics went away. There are a number of http related changes between Go 1.17 and Go 1.17.5:
https://github.com/golang/go/commit/01779135c09ac4c2ba0d0bc7b3ae0889ad9ad989 https://github.com/golang/go/commit/ab175939e28df62fee09c33c7d9e008bff6b684c https://github.com/golang/go/commit/f9cb33c7c9d48fd0c38d17a659d5e968de648d40
Looking at the stack trace, The most interesting bit is
bytes.(*Reader).Read
. Thenet/http
stack is callingRead()
on a*bytes.Reader
that is nil.In libhoney, traces are accumulated and then compressed with zstd. For pooling purposes, libhoney wraps a buffer in a
pooledReader
, which embeds a*bytes.Reader
. When the pooledReader gets a Close, it sets the embedded*bytes.Reader
to nil. So we know that any call to pooledReader.Read after pooledReader.Close will panic. It's not clear from the interface documentation of Reader and Closer whether such behavior is allowed.However, net/http's RoundTripper says:
Note that this says the request must not be reused until the Response's Body has been closed. I think the current implementation of pooledReader is violating that. I haven't been able to come up with a minimal repro of a case where net/http calls Read after Close, but the combination of things here makes me suspect an issue in libhoney-go.
One possible fix would be to provide a
.rePool()
function onpooledReader
, and call it explicitly afterhttpClient.Do(req)
returns.