proposal: io: allocate copy buffers from a pool #58452
Comments
|
The |
|
Using the race detector sounds like a good idea. I wonder if we could use it more broadly for Write calls: the compiler could instrument call sites to detect bad implementations, such as:
It would add some overhead for sure, but it would cover writes happening outside |
|
Change https://go.dev/cl/466865 mentions this issue: |
I don't think we should do that as a compiler transformation, because in general a Go program might have side-channel information about an implementation and the compiler's job is only to implement the language spec. That said, it does seem easy to implement an We could use that implementation under the race detector in libraries like |
When the race detector is enabled, scribble over copy buffers with garbage after Write returns. For #58452 Change-Id: I25547684bcbef7d302d76736cb02e59c89a640ee Reviewed-on: https://go-review.googlesource.com/c/go/+/466865 Reviewed-by: Bryan Mills <bcmills@google.com> Run-TryBot: Damien Neil <dneil@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Damien Neil <dneil@google.com>
dsnet
changed the title
|
I like that idea. Reminds me of https://pkg.go.dev/testing/iotest. This pattern could be generalised to other similar interfaces, with parameters documented as "must not be retained". That's why my mind was going for the compiler - if it knew about those one way or another, it could enable the extra checks on |
|
The change introduced a buffer allocation optimization using a sync.Pool, but this resulted in a race condition in the HTTP/2 ResponseWriter. The suggested fix involves detecting io.Writers that retain the buffer and invalidating buffer contents after each Write call. |
Shouldn't it check the hash after write call is complete? Or do I miss the idea? |
When the race detector is enabled, scribble over copy buffers with garbage after Write returns. For golang#58452 Change-Id: I25547684bcbef7d302d76736cb02e59c89a640ee Reviewed-on: https://go-review.googlesource.com/c/go/+/466865 Reviewed-by: Bryan Mills <bcmills@google.com> Run-TryBot: Damien Neil <dneil@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Auto-Submit: Damien Neil <dneil@google.com>
https://go.dev/cl/456555 changed
io.Copyto allocate its copy buffers from async.Pool. This exposed a race condition in the HTTP/2ResponseWriter#58446, which can be fixed, but I'm taking this as a sign that any change should be made more judiciously; https://go.dev/cl/467095 will revert that change.io.Copy, prior to CL 456555, allocates a 32k[]bytebuffer (or a smaller one if the source is an*io.LimitedReaderwith a size less than 32k). Allocating these buffers from a pool reduces GC allocations and notably improvesio.Copyperformance; see benchmarks in that CL's description. Thenet/httppackage maintains its own pool of copy buffers for this reason. Having every package that usesio.Copymaintain its own buffer pool seems unfortunate.If the destination of a
Copyretains the buffer passed to theWritecall after returning, this results in a race condition where the buffer may be recycled through the pool while theWriteis still in progress. This behavior is a violation of theio.Writercontract, but as #58446 demonstrates, this does happen in practice.Perhaps we should start by detecting
io.Writers that retain the buffer. When the race detector is enabled,io.Copycould write over the copy buffer after each call toWrite. This will help the race detector to identify cases where another goroutine continues to access the buffer, as well as invalidating the buffer contents. https://go.dev/cl/466865 contains this change.The text was updated successfully, but these errors were encountered: