proposal: bufio: add Reader.Buffer #50960
Comments
|
The workaround does not work in a situation where temporary errors are expected as you can't "unread" the buffer. This is one of the reasons why |
|
Reader has a fixed size buffer set at the beginning of time. I can't quite tell which of a few things you are asking for:
I am assuming you don't want to use bufio.NewReaderSize(r, 64<<20) followed by r.Reset to reuse the buffer, because you don't want every Reader to be sitting on 64 MB? |
Yes.
No. The proposed
Exactly. Start with something small (e.g., 4KiB) and permit growing up to some maximum specified limit. |
|
This proposal has been added to the active column of the proposals project |
|
Do you think you need the ability to set the initial slice yourself, or are you OK with bufio always allocating it? If we don't need the ability to set the slice ourselves, maybe we could
Do either of those work for you? Any thoughts on which is better? |
Our use case didn't care about setting the slice. I've personally never used the "set the slice" feature in
Both options work for us. Option 2 is more flexible as it allows modifying the buffer size after the fact, but then you'll need to deal with what happens when the buffer is set to be smaller than what's currently utilized. |
|
Let's figure out whether we can do without API changes, since that's always preferable. Any reasons not to take this approach? |
|
That approach SGTM, but this behavior is potentially observable through the I can make a prototype CL and we can run through global testing and see how many things break and whether it's legitimate. |
|
Sounds good, thanks Joe. |
|
Waiting on a prototype CL from Joe to see if we can get away without an API change here. |
|
Placing on hold for prototype CL. Please feel free to move out of hold once it is ready. |
|
Placed on hold. |
The
Readertype is initialized with a buffer size. Once set, there is no API to adjust this.This can make it challenging when parsing a file with a combination of
PeekandDiscardcalls.One such pattern is something like:
The code above works just fine, but fails unexpectedly when the payload size exceeds the internal buffer of
Reader.The current workaround is to check for
ErrBufferFull, allocate a separate buffer, and read the expected number of bytes into that buffer. The workaround looks like:var r *bufio.Reader = ... for { ... buf, err := r.Peek(hdr.Size) - if err != nil { - return err - } - r.Discard(len(buf)) + switch { + case err == nil: + r.Discard(len(buf)) + case err == bufio.ErrBufferFull: + buf = make([]byte, hdr.Size) + if _, err := io.ReadFull(r, buf); err != nil { + return err + } + default: + return err + } processData(buf) }It is unfortunate that I need to allocate a relatively large buffer in order to read large records and that I'm not able to reuse that buffer for subsequent
Readeroperations.I propose the addition of
Reader.Buffer, with similar semantics asScanner.Buffer:Using this new API, the code above would look like:
var r *bufio.Reader = ... + r.Buffer(nil, 64<<20) // we never expect records to exceed 64MiB for { ... }\cc @josharian
The text was updated successfully, but these errors were encountered: