fix: correctly return number of bytes read from chunked streams #1386
Conversation
This comment has been minimized.
This comment has been minimized.
| @@ -47,11 +47,12 @@ abstract class AwsChunkedByteReadChannelTestBase : AwsChunkedTestBase(AwsChunked | |||
| val sink = SdkBuffer() | |||
|
|
|||
| val bytesRead = awsChunked.readAll(sink) | |||
| writeJob.join() | |||
| // writeJob.join() | |||
There was a problem hiding this comment.
Do we still need to join the write job?
There was a problem hiding this comment.
It doesn't hurt but it's not strictly necessary. The readAll call cannot return until all the data's been consumed which means the write job will have finished. I've uncommented that line just for a sanity check.
| "issues": [ | ||
| "awslabs/smithy-kotlin#1285" | ||
| ], | ||
| "requiresMinorVersionBump": true |
There was a problem hiding this comment.
Adding a note so we don't forget to remove requiresMinorVersionBump
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| consumeAndWriteChunk(destination) | ||
| } | ||
| if (rc == -1L) { | ||
| check(input.eof) { "Got back -1 bytes but input adapter is not EOF" } |
There was a problem hiding this comment.
When would the rc be -1 but the input not eof?
There was a problem hiding this comment.
This is mostly a sanity check. It was useful while debugging the implementation but could provide a valuable fail-fast clue if a future bug involves streams which return -1 bytes read but the stream is not yet closed/exhausted.
| if (hasLastChunkBeenSent) { | ||
| return -1 // this stream is exhausted | ||
| } else { | ||
| // Write any remaining data as final chunk(s) |
There was a problem hiding this comment.
I think we would only have one final chunk in the buffer at this point not multiple?
There was a problem hiding this comment.
You're right, corrected the comment.
Affected ArtifactsChanged in size
|
Issue #
Fixes #1285
Description of changes
This change updates
AwsChunkedSourceandAwsChunkedByteReadChannelto return the actual number of bytes read from the underlying stream, not the number of bytes written to the sink. It does this by adding read count tracking toAwsChunkedReaderwhich must be read and reset by the calling source/channel. Also updates various tests to correctly assert the number of bytes read and the number of bytes written.This PR is an alternate implementation of #1294.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.