Skip to content

Add checksum listeners before response targets #3250

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

Merged
merged 10 commits into from
May 28, 2025
Merged

Conversation

mullermp
Copy link
Contributor

@mullermp mullermp commented May 26, 2025

Fixes #3244

This approach simply emits the chunk data prior to writing it out to the body. When emitting, the chunk may be modified, then that modified chunk may be written to the body. Checksum verification relies on on_data to verify chunks of data. Checksum verification fails when the chunk is mutated.

Initially tried a different approach of moving moving checksum response listeners to be very early in the stack (listeners are executed in the order that they are registered) and changing the listeners to use on_data instead of on_headers, but this had issues because on_data would re-register bodies each time and had issues with client encryption.

@mullermp mullermp requested a review from jterapin May 26, 2025 15:30
Copy link

github-actions bot commented May 26, 2025

Detected 1 possible performance regressions:

  • aws-sdk-cloudwatchlogs.require_time - z-score regression: 3.32 -> 13.6. Z-score: 22.67

@mullermp mullermp marked this pull request as draft May 26, 2025 17:18
@mullermp mullermp marked this pull request as ready for review May 26, 2025 18:13
Copy link
Contributor

@jterapin jterapin left a comment

Choose a reason for hiding this comment

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

Nice, looks fine. I added some minor alignment suggestions so that it's easier to resolve it here.

Copy link
Contributor

@alextwoods alextwoods left a comment

Choose a reason for hiding this comment

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

Nice - approach makes sense.

Co-authored-by: Juli Tera <57973151+jterapin@users.noreply.github.com>
@jterapin jterapin merged commit b359203 into version-3 May 28, 2025
34 checks passed
@jterapin jterapin deleted the checksum-listener branch May 28, 2025 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Aws::S3::Plugins::NonRetryableStreamingError/Aws::Errors::ChecksumError in get_object streaming
3 participants