Repository navigation
Handle short reads in concurrent File.ReadAt - #697
christhomas wants to merge 1 commit into
Conversation
|
Implemented the approach discussed in #677. The client now reads the first chunk synchronously and uses the returned byte count as the size for subsequent concurrent reads, which accommodates servers that cap individual reads below the client maximum. When a concurrent chunk is short, it probes the exact next offset: EOF confirms the read ended, while more data returns io.ErrUnexpectedEOF so the caller is not given a silently truncated result. |
a56b45d to
7864c83
Compare
| for err == nil && n < len(b) { | ||
| id := f.c.nextID() | ||
| typ, data, err := f.c.sendPacket(context.Background(), ch, &sshFxpReadPacket{ | ||
| ID: id, | ||
| Handle: f.handle, | ||
| Offset: uint64(off) + uint64(n), | ||
| Len: uint32(len(b) - n), | ||
| }) | ||
| if err != nil { | ||
| return n, err | ||
| } | ||
| var read int | ||
| read, err = f.readOnceAt(ch, b[n:], off+int64(n)) | ||
| n += read | ||
| } |
There was a problem hiding this comment.
🤔 If we refactor this a bit, we avoid needing to separately declare the read variable…
for n < len(b) {
read, err := f.readOnceAt(ch, b[n:], off+int64(n))
n += read
if err != nil {
return read, err
}
}
return n, nilThere was a problem hiding this comment.
I simplified readChunkAt so each loop iteration reads into the remaining buffer and advances the cumulative count. If a later read returns an error, it returns the total bytes read so far rather than just that iteration’s count. The regression tests for short replies and partial reads followed by EOF continue to cover that behavior.
| // A short read can be an EOF or a server that returned | ||
| // less data than requested. Probe the exact next offset to | ||
| // distinguish those cases before reporting EOF. | ||
| err = f.probeForEOF(packet.off + int64(n)) |
There was a problem hiding this comment.
🤔 actually, I was just thinking… instead of probing for EOF, we could just run the sequential readAt instead… that should properly handle not just a proper EOF case, but also the case where a file spontaneously continues after the apparent EOF…
Maybe not as important here, since the buffer input is probably shorter than most concurrent reads are going to do, but it would be useful on WriteTo.
There was a problem hiding this comment.
I removed the one-byte EOF probe. When a concurrent chunk is short, readAt now calls readAtSequential on the unread portion of that chunk, starting at the exact next offset. That lets the read continue through server-capped short replies and detect data beyond an apparent EOF; it reports EOF only if the sequential read actually reaches it. I added a regression test for completing a read across short concurrent replies; go test ./... and go test -race ./... both pass.
Read the first chunk synchronously and size concurrent requests from the server response. When concurrent reads are short, complete the remainder sequentially instead of probing for EOF, so reads handle capped responses and data that appears beyond an apparent EOF. Keep cumulative byte counts when readChunkAt retries short responses, and add regression coverage for concurrent short replies. Fixes pkg#677.
c3567a8 to
f4a8e08
Compare
Fixes #677. Read the first chunk before starting concurrent reads so the client can adapt later chunk sizes to servers that return less data than requested. Probe the exact offset after a short chunk: EOF confirms the file end, while additional data returns io.ErrUnexpectedEOF instead of silently truncating the read.