Repository navigation
objectstorage: fail a ranged read cut short by cancellation - #3049
Benjamin Elder (BenTheElder) merged 2 commits into
Conversation
When ctx was done, schedule closed ordered and Read returned io.EOF, even with bytes of the object still unread. A plain zstd stream cut at a frame boundary then decoded as a clean, truncated success. Read now returns the context's cause when it got fewer than size bytes.
| size int64 | ||
| // read is how many bytes Read has taken from ordered. | ||
| read int64 | ||
| ctx context.Context |
There was a problem hiding this comment.
Ideally context should be passed in not placed in a structure. https://go.dev/blog/context-and-structs explains why this is a bad idea.
There was a problem hiding this comment.
Agree, and claude has a suggestion that I like for avoiding that.
There was a problem hiding this comment.
Fixed in 702186c with Ben's suggestion. rangedReader no longer holds ctx.
| r.err = io.EOF | ||
| if r.read < r.size { | ||
| // schedule stops early only when ctx is done. | ||
| r.err = fmt.Errorf("ranged read stopped after %d of %d bytes: %w", r.read, r.size, context.Cause(r.ctx)) |
There was a problem hiding this comment.
🤖 nit 🟢 – This could drop both ctx and read from the struct, which also answers your note about ctx in a struct. schedule already knows when it stops early, so it can set e.g. r.stopErr = context.Cause(ctx) in its two ctx.Done() branches, and Read returns that instead of io.EOF once ordered is closed. The deferred close(r.ordered) happens before the receive that sees it closed, so reading the field is race-free.
https://go.dev/ref/mem#chan
There was a problem hiding this comment.
Done in 702186c. schedule sets stopErr in both ctx.Done branches, and Read returns it once ordered is closed. ctx and the read counter are gone from rangedReader.
| size int64 | ||
| // read is how many bytes Read has taken from ordered. | ||
| read int64 | ||
| ctx context.Context |
There was a problem hiding this comment.
Agree, and claude has a suggestion that I like for avoiding that.
Benjamin Elder (BenTheElder)
added
kind/bug
schedule sets stopErr when ctx is done before every range is issued. Read returns it once ordered is closed. This drops ctx and the byte count from rangedReader.
Benjamin Elder (BenTheElder)
left a comment
Benjamin Elder (BenTheElder)
left a comment
There was a problem hiding this comment.
no claude, just me. this LGTM. thanks.
Why is this needed?
When ctx was done, schedule closed ordered and Read returned io.EOF, even with bytes of the object still unread. A plain zstd stream cut at a frame boundary then decoded as a clean, truncated success. schedule now records the context's cause when it stops early, and Read returns it instead of io.EOF.
Fixes #3047
AI use: Claude assisted.