Skip to content

fix: extra byte read from chunk transfer #1294

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

xinsong-cui
Copy link
Contributor

@xinsong-cui xinsong-cui commented Jun 9, 2025

Issue #

Fixes #1285

Description of changes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

This comment has been minimized.

This comment has been minimized.

@xinsong-cui xinsong-cui marked this pull request as ready for review June 9, 2025 17:35
@xinsong-cui xinsong-cui requested a review from a team as a code owner June 9, 2025 17:35

This comment has been minimized.

This comment has been minimized.

Comment on lines 50 to 60
return chunkReader.chunk.read(sink, limit)

val totalBytesTransferred = chunkReader.chunk.read(sink, limit)
contentBytesTransferred = totalBytesTransferred - chunkReader.chunkMetadataBytes

return totalBytesTransferred
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: I feel pretty strongly that AwsChunkedSource.read should return the number of bytes which were read from the delegate source rather than the number of bytes written to the destination sink. Can we just return totalBytesTransferred - chunkReader.chunkMetadataBytes instead of storing it in a property? What do we need the total bytes transferred for (at least, as the output from read)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to return totalBytes for some of our tests:

val totalBytesExpected = encodedUnsignedChunkLength(CHUNK_SIZE_BYTES) + encodedUnsignedChunkLength(0) + "\r\n".length

If we not return the totalBytesTransferred, those tests will fail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure we can find a way around that, maybe by exposing the total bytes as some kind of test utility?

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@xinsong-cui xinsong-cui added the no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly. label Aug 8, 2025

This comment has been minimized.

@xinsong-cui
Copy link
Contributor Author

Manually added no-changelog label as changelog check somehow not passing

Copy link

github-actions bot commented Aug 8, 2025

Affected Artifacts

No artifacts changed size

Comment on lines +48 to +52
/**
* Gets the total bytes transferred for testing purposes only.
*/
@InternalApi
public fun getTotalBytesTransferred(): Long = totalBytesTransferred
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think adding a public API "for testing purposes only" is the best. Can we refactor this to make it internal-only, or otherwise refactor the tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Progress listening with chunked encoding reads more bytes transferred than file size
3 participants