Skip to content
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

Feature: add support for aws-chunked content encoding #1501

Merged
merged 15 commits into from
Jul 5, 2022
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 34 additions & 6 deletions aws/rust-runtime/aws-http/src/content_encoding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,11 +239,17 @@ where
return match this.inner.poll_trailers(cx) {
Poll::Ready(Ok(trailers)) => {
*this.state = AwsChunkedBodyState::Closed;
let expected_total_trailer_length =
total_rendered_length_of_trailers(trailers.as_ref());
let actual_total_trailer_length = this.options.total_trailer_length();
assert_eq!(expected_total_trailer_length, actual_total_trailer_length,
"while writing trailers, the expected length of trailers ({expected_total_trailer_length}) differed from the actual length ({actual_total_trailer_length})");
let expected_length = total_rendered_length_of_trailers(trailers.as_ref());
let actual_length = this.options.total_trailer_length();

if expected_length != actual_length {
let err =
Box::new(AwsChunkedBodyError::ReportedTrailerLengthMismatch {
actual: actual_length,
expected: expected_length,
});
return Poll::Ready(Some(Err(err)));
}

let mut trailers = trailers_as_aws_chunked_bytes(trailers);
// Insert the final CRLF to close the body
Expand Down Expand Up @@ -276,6 +282,28 @@ where
}
}

/// Errors related to `AwsChunkedBody`
#[derive(Debug)]
enum AwsChunkedBodyError {
/// Error that occurs when the sum of `trailer_lengths` set when creating an `AwsChunkedBody` is
/// not equal to the actual length of the trailers returned by the inner `http::Body`
/// implementor. These trailer lengths are necessary in order to correctly calculate the total
/// size of the body for setting the content length header.
ReportedTrailerLengthMismatch { actual: u64, expected: u64 },
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see @rcoh suggested making an error for this, but I think this is a case where we have a bug in our code, and there's little to no value for a customer in matching on this error. It's a case that simply should not happen if the code is correct, so an assertion was warranted.

On the flip side, with an assertion, it would silently allow the lengths to mismatch when compiled in release mode, which would likely result in a server-side error response. I think this is OK though, since a bug with this should not have been released in the first place if it was adequately tested.

We might want to proptest header names/values for the size prediction vs. the rendering to be absolutely certain they cannot mismatch. I think they can right now in the case where a header value has Unicode characters in it since the length predictor function is using string length instead of byte length.

Copy link
Collaborator

@jdisanti jdisanti Jun 29, 2022

Choose a reason for hiding this comment

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

Another thing we could consider is just getting rid of the length prediction entirely. Just render the trailers to a BytesMut when the length is initially needed, and store that BytesMut in the AwsChunkedBody for later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jdisanti only debug_assert doesn't fail in release mode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a new error for when stream_length is set incorrectly

}

impl std::fmt::Display for AwsChunkedBodyError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
Self::ReportedTrailerLengthMismatch { actual, expected } => {
write!(f, "When creating this AwsChunkedBody, length of trailers was reported as {expected}. However, when double checking during trailer encoding, length was found to be {actual} instead.")
}
}
}
}

impl std::error::Error for AwsChunkedBodyError {}

// Used for finding how many hexadecimal digits it takes to represent a base 10 integer
fn int_log16<T>(mut i: T) -> u64
where
Expand Down Expand Up @@ -341,7 +369,7 @@ mod tests {
}

#[tokio::test]
#[should_panic = "assertion failed: `(left == right)`\n left: `0`,\n right: `42`: while writing trailers, the expected length of trailers (0) differed from the actual length (42)"]
#[should_panic = "called `Result::unwrap()` on an `Err` value: ReportedTrailerLengthMismatch { actual: 42, expected: 0 }"]
async fn test_aws_chunked_encoding_incorrect_trailer_length_panic() {
let input_str = "Hello world";
// Test body has no trailers, so this length is incorrect and will trigger an assert panic
Expand Down