Add io.BufferedReader.peek (fixes #27915) - #27928
Conversation
cc59786 to
047701f
Compare
einar-hjortdal
changed the title
|
Thanks for your contribution! When you open a PR, please describe the fix you made, the expected behavior, and the validation you performed. This helps Codex take your explanations into account when we trigger a review. If you can edit the initial description of your PR to include this information, please do. Thanks! |
|
Out of scope but I think returning |
Ok sounds good, sorry I knew nothing of Codex |
|
Thanks. Before review, a few cases still need fixing. Local probes showed that Please add
Once these cases are fixed and the regression tests pass, I’ll ask Codex to review the PR. Thanks! |
|
Should be good |
|
Thanks. I ran the current tests and also added temporary probes using a real EOF reader and partial reads. Two issues remain:
The current ArrayReader hides these cases because it returns 0 instead of Eof and always copies as much data as possible. Please also cover temporary zero-length reads, read_line after EOF, n larger than the buffer capacity, and make the negative-n test fail if no error is returned. Once these cases pass, I will launch the Codex review. Thanks! |
I'm sorry I don't understand how you got that to happen, I can't replicate it
Yes this is because I did not implement the buffer refilling logic as a loop. I can do that and it'll fix the problem. This may be a problem for the BufferedReader.read method too actually, it's kind of going out of scope of my pull request, but I'll take a look at it 😭 |
|
I reproduced it on the current PR head with this minimal reader: struct EofAfterDataReader {
data []u8
mut:
offset int
}
fn (mut r EofAfterDataReader) read(mut buf []u8) !int {
if r.offset >= r.data.len {
return io.Eof{}
}
n := copy(mut buf, r.data[r.offset..])
r.offset += n
return n
}
mut source := &EofAfterDataReader{
data: 'abc'.bytes()
}
mut reader := io.new_buffered_reader(reader: source, cap: 4)
assert reader.peek(4)! == 'abc'.bytes()The assertion currently fails because The initial read stores 3 bytes, then the refill asks for one more and receives You are right about the one-byte reader: the refill needs to loop until the request is satisfied, EOF is reached, or no further progress can be made. Please keep any |
I'm trying not to touch anything else, but maybe an issue should be opened about the read method |
|
Yes, keeping this PR focused on |
|
Please open a separate issue for |
|
The failing test also fails on master. That's because the read method returns as soon as it gets any data instead of looping until buffer is full, eof or failure. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c005baab5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| r.offset = 0 | ||
|
|
||
| for r.len < r.buf.len { |
There was a problem hiding this comment.
When a streaming reader returns short reads, this loop continues until the entire buffer is full rather than stopping once the requested n bytes are available. With the default 128 KiB capacity, a small peek can therefore perform many unnecessary reads or block indefinitely waiting for unrelated future socket data; pass the requested target into this helper and stop when either that target or capacity is reached.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I do not believe this is a concern right now. This is a feature that can be added later. Buffer capacity of a buffered reader is configurable, users already expect such reads would be done.
| // returns Eof if reached the end of the stream. | ||
| // returns an error if n < 0. | ||
| // returns an array with less than n bytes if n > capacity. | ||
| pub fn (mut r BufferedReader) peek(n int) ![]u8 { |
There was a problem hiding this comment.
This adds BufferedReader.peek as a public standard-library API, but the commit leaves vlib/io/README.md unchanged, so the module documentation does not describe the new operation or its partial-result/EOF behavior. The repository explicitly requires public API additions to update the module docs or README.
AGENTS.md reference: AGENTS.md:L219-L222
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcc0753423
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| r.offset = 0 | ||
|
|
||
| for r.len < r.buf.len { |
There was a problem hiding this comment.
When the underlying Reader returns short chunks and the initial fill leaves fewer than n bytes, this loop continues reading until the entire buffer is full rather than stopping once n bytes are available. With the default 128 KiB buffer on a live network connection, peek(2) can issue another blocking read after two bytes have already been buffered and wait for unrelated future traffic; refill only up to min(n, capacity).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
fill_buffer_keep_unread is meant to fill the buffer for the next reads. The logic isn't that different fromfill_buffer. If fill_buffer hangs, so will fill_buffer_keep_unread.
hanging is handled by the underlying reader, such as with tcpconn set_read_timeout.
the purpose of the buffer is to have data ready for the next read, not to read data only when a read is needed.
This suggestion is plain dumb 🤷♂️
| return error('cannot peek a negative number of bytes') | ||
| } | ||
|
|
||
| if r.needs_fill() { // has no unread bytes |
There was a problem hiding this comment.
When n == 0 and there are no unread bytes, this branch calls fill_buffer() before reaching the zero-length slice return. Consequently, peek(0) returns Eof for an empty reader and can block or consume input from a live reader, even though no bytes were requested; it should return an empty array without touching the upstream reader.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
not in scope: BufferedReader is not thread-safe by itself, users have to handle race conditions with mutex etc.
3 participants
Fix #27915
Lacks tests, looking for feedback, please take over this pull request.
Adds peek method: this method reads n bytes from the buffer without advancing the cursor (offset). Unlike read, it returns a clone of the byte slice.
This method refills the buffer if there is not enough data to satisfy the request.
Handles when the requests asks for too much data: more than the buffer can hold, and more than the reader can read.
Refactors the read method to read bytes even though the enf of the stream has been reached: this is because peek may refill the buffer and encounted the end of stream, a situation that wasn't possible before.
This method should not lose any unread data from the internal buffer, it can discard read data to get more unread data. It should return the exact number of u8 requested, or less when reaching eof or when n is larger than the internal buffer capacity.