-
Notifications
You must be signed in to change notification settings - Fork 2.2k
enhancement(sources): pre-size HTTP request body buffer from Content-Length #25747
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
base: master
Are you sure you want to change the base?
Changes from all commits
bbf2140
2c6c5af
428db8b
3e16af7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,13 +60,12 @@ pub(crate) fn limited_body(max_body_size: usize) -> BoxedFilter<(Bytes,)> { | |
| max_body_size, | ||
| ))) | ||
| } else { | ||
| Ok(()) | ||
| Ok(declared) | ||
| } | ||
| }) | ||
| .untuple_one() | ||
| .and(warp::body::stream()) | ||
| .and_then(move |body| async move { | ||
| collect_body_with_limit(body, max_body_size) | ||
| .and_then(move |declared: Option<u64>, body| async move { | ||
| collect_body_with_limit(body, max_body_size, declared) | ||
| .await | ||
| .map_err(warp::reject::custom) | ||
| }) | ||
|
|
@@ -178,19 +177,35 @@ fn decompress_snappy( | |
| Ok(decoded.into()) | ||
| } | ||
|
|
||
| /// Cap on the initial buffer allocation when pre-sizing from a `Content-Length` header. | ||
| /// Prevents stalling clients from reserving large amounts of memory before sending any bytes. | ||
| #[cfg(any( | ||
| feature = "sources-utils-http-prelude", | ||
| feature = "sources-opentelemetry", | ||
| test | ||
| ))] | ||
| async fn collect_body_with_limit<S, B>(body: S, max_body_size: usize) -> Result<Bytes, ErrorMessage> | ||
| const MAX_INITIAL_BODY_CAPACITY: usize = 8 * 1024 * 1024; // 8 MiB | ||
|
|
||
| #[cfg(any( | ||
| feature = "sources-utils-http-prelude", | ||
| feature = "sources-opentelemetry", | ||
| test | ||
| ))] | ||
| async fn collect_body_with_limit<S, B>( | ||
| body: S, | ||
| max_body_size: usize, | ||
| declared_len: Option<u64>, | ||
| ) -> Result<Bytes, ErrorMessage> | ||
| where | ||
| S: futures_util::Stream<Item = Result<B, warp::Error>>, | ||
| B: Buf, | ||
| { | ||
| futures_util::pin_mut!(body); | ||
|
|
||
| let mut bytes = BytesMut::new(); | ||
| let capacity = declared_len | ||
| .and_then(|len| usize::try_from(len).ok()) | ||
| .map_or(0, |len| len.min(max_body_size).min(MAX_INITIAL_BODY_CAPACITY)); | ||
| let mut bytes = BytesMut::with_capacity(capacity); | ||
|
Comment on lines
+205
to
+208
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an unauthenticated HTTP client sends a Useful? React with 👍 / 👎.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. IMO, it's acceptable given the performance regression that the alternative causes.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd agree with its suggestion to cap the initial capacity at some suitably large but smaller-than-100MiB number. That would minimize still reallocations, just not eliminate them.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. I set 8 MB somewhat arbitrarily some Vector constants are around these values: Kinesis Firehose: 4 MiB, syslog recv buffer: 4 MiB, Windows event log: 10 MiB), does that seem reasonable? |
||
| while let Some(chunk) = body.next().await { | ||
| let chunk = chunk.map_err(|error| { | ||
| ErrorMessage::new( | ||
|
|
@@ -368,7 +383,7 @@ mod tests { | |
| Ok::<_, warp::Error>(Bytes::from_static(b" world")), | ||
| ]); | ||
|
|
||
| let collected = collect_body_with_limit(body, 11).await.unwrap(); | ||
| let collected = collect_body_with_limit(body, 11, Some(11)).await.unwrap(); | ||
| assert_eq!(collected, Bytes::from_static(b"hello world")); | ||
| } | ||
|
|
||
|
|
@@ -379,7 +394,7 @@ mod tests { | |
| Ok::<_, warp::Error>(Bytes::from_static(b" world")), | ||
| ]); | ||
|
|
||
| let err = collect_body_with_limit(body, 5) | ||
| let err = collect_body_with_limit(body, 5, None) | ||
| .await | ||
| .expect_err("should reject"); | ||
| assert_eq!(err.status_code(), StatusCode::PAYLOAD_TOO_LARGE); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edge case: if
VECTOR_MAX_DECOMPRESSED_SIZE_BYTES/--max-decompressed-size-bytesis set to a value < 8MiB we should honor it by not allocating more than32 + max_size + max_size / 6(this is snappy's worst-case scenario, which is larger than gzip, zstd and zlib's worst-case scenarios)