services/object: fix read complex objects regressions #3568
Conversation
End-rey
requested review from
carpawell,
cthulhu-rider and
roman-khimov
as code owners
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3568 +/- ##
==========================================
- Coverage 25.98% 25.93% -0.06%
==========================================
Files 660 660
Lines 49738 49729 -9
==========================================
- Hits 12926 12898 -28
- Misses 35775 35792 +17
- Partials 1037 1039 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
End-rey
force-pushed
the
fix-stream-problems
branch
6 times, most recently
from
daa80da to
847d5a4
Compare
|
Multiple focused commits (in a single PR) would be easier to digest, there are different problems being solved here. |
pkg/services/object/get/exec.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("read next payload chunk: %w", err) | ||
| } | ||
| if n == 0 { |
There was a problem hiding this comment.
why is this necessary?
There was a problem hiding this comment.
It fix problem with s3 uploading multipart objects. Idk why, but in this case, a reader appears that reads zero bytes without errors, resulting in an infinite loop.
There was a problem hiding this comment.
Without an error? This shouldn't happen. Likely this means some reader implementation is broken in our code and we're not returning io.EOF properly.
There was a problem hiding this comment.
This happens when the payload size is zero and the buffer size then becomes zero
neofs-node/pkg/services/object/get/exec.go
Line 346 in 9306bda
And here the result is returned without EOF
https://github.com/nspcc-dev/neofs-sdk-go/blob/666fbf83b88bbbb51b7d0ef6d5ecf371e1435231/client/object_get.go#L196-L202
There was a problem hiding this comment.
Implementations of Read are discouraged from returning a zero byte count with a nil error, except when len(p) == 0. Callers should treat a return of 0 and nil as indicating that nothing happened; in particular it does not indicate EOF.
https://go.dev/play/p/PqARo45a-Xv
Looks like it's a caller problem, if it knows the size is zero it should not call Read(). If it doesn't know the size (it's a stream after all), it should pass some buffer and get EOF. Now the question is where these rules should be applied in our codebase, we have a number of different readers. But just treating n == 0 as a signal of its own here seems wrong to me.
There was a problem hiding this comment.
passing empty buffer is nonsense. The easiest thing to do is max(x, 1).
There was a problem hiding this comment.
If bufSize is 0 (and if this only happens when payload is zero) then we can skip the reader loop altogether, just return nil.
There was a problem hiding this comment.
this'd be another change cuz explicit reading also checks that exactly PayloadSize() bytes are streamed, 0 in particular
End-rey
force-pushed
the
fix-stream-problems
branch
from
847d5a4 to
9306bda
Compare
Fix problems with writing children logic with new stream logic. Refs #3562. Signed-off-by: Andrey Butusov <[email protected]>
pkg/services/object/get/exec.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("read next payload chunk: %w", err) | ||
| } | ||
| if n == 0 { |
There was a problem hiding this comment.
Without an error? This shouldn't happen. Likely this means some reader implementation is broken in our code and we're not returning io.EOF properly.
End-rey
force-pushed
the
fix-stream-problems
branch
from
9306bda to
773c361
Compare
End-rey
force-pushed
the
fix-stream-problems
branch
3 times, most recently
from
1432be2 to
851d110
Compare
Fix that the range request with new stream logic don't fall back to GET after `apistatus.ErrObjectAccessDenied`. Refs #3562. Signed-off-by: Andrey Butusov <[email protected]>
Fix an infinite loop while reading from the reader if the size of the written bytes is zero and there is no error. The buffer size can no longer be 0. Closes #3562. Signed-off-by: Andrey Butusov <[email protected]>
Closes #3562.