Use Body::size_hint to determine whether a request is retryable by olix0r · Pull Request #1345 · linkerd/linkerd2-proxy

olix0r · 2021-10-31T02:09:25Z

Currently, we look at each request's content-length header to
determine whether a request is too large to buffer for retries. This is
unnecessary, as hyper will already read the content-length and use it
populate the Body::size_hint value.

This change modifies the ReplayBody constructor (now
ReplayBody::try_new) to check the body's size hint and fails to
construct a ReplayBody if the lower bounds of the body size exceeds
the maximum buffer size.

In doing so, we update all Body implementations to ensure that
size_hint is properly implemenented. We also update the
ReplayBody::size_hint implementation to be more robust, returning the
original size hint if a clone of ReplayBody does not currently hold
the inner body.

This change rewrites `RetryPolicy::can_retry` as `RetryPolicy::body_too_big` and updates it to avoid reading `content-length` header, instead relying on the `Body::size_hint` value (which is set by the HTTP server if a `content-length` header is present. Renaming the function helps us express what the function actually checks to help disambiguate it from all of the other 'retry' related terminology in this file and is more explicit about what it does: we don't precicesly know if the request is retryable, rather we know whether the request is obviously too large to be retried. This also lets us simplify comments and logging as the flow is more self-explanatory. This change also adds unit tests for `RetryPolicy::body_too_big` to exercise the variety of scenarios this function handles.. Furthermore, this change ensures that all `Body` implementations correctly proxy `Body::size_hint`. `ReplayBody` is updated to cache the the inner body's size hint value in the shared state so it can be reused across all clones of the `ReplayBody` regardless of whether the current clone holds the body state or not.

…length... but that seems okay

olix0r · 2021-10-31T03:03:26Z

linkerd/app/integration/src/tests/transparency.rs

-                    let has_body_header = req.headers().contains_key("transfer-encoding")
-                        || req.headers().contains_key("content-length");


I have no idea why this test--which only mentions transfer-encoding in its description--also checks for content-length... Now that we're properly preserving the size hint, we can set the content-length header to 0 if we know it's empty. This seems like a strictly good thing.

Currently, we look at each request's `content-length` header to determine whether a request is too large to buffer for retries. This is unnecessary, as hyper will already read the `content-length` and use it populate the `Body::size_hint` value. This change modifies the `ReplayBody` constructor (now `ReplayBody::try_new`) to check the body's size hint and fails to construct a `ReplayBody` if the lower bounds of the body size exceeds the maximum buffer size. In doing so, we update all `Body` implementations to ensure that `size_hint` is properly implemenented. We also update the `ReplayBody::size_hint` implementation to be more robust, returning the original size hint if a clone of `ReplayBody` does not currently hold the inner body.

olix0r · 2021-11-01T16:29:45Z

linkerd/http-retry/src/lib.rs

-                .as_ref()
-                .map(|rest| {
-                    let hint = rest.size_hint();
-                    (hint.lower(), hint.upper().unwrap_or(0))


I think that hint.upper().unwrap_or(0) is a subtle bug -- if the lower bound is non-zero, we'll end up having an upper limit that is less than the lower limit, which is bad. It's better to omit the upper limit if the inner body does not specify it.

hawkw

yup, this seems good to me --- i hadn't realized that size_hint would be set from the request's content-length; i had assumed it was just the size of any currently-received body chunks. this definitely seems like the right approach since it will also let us make better decisions in the case where the whole body has been received but it didn't have a content-length.

i had some minor nits and suggestions, but no hard blockers --- let me know what you think?

hawkw · 2021-11-01T17:02:18Z

linkerd/app/core/src/retry.rs

-        }
-        Either::B(req)
+        let (head, body) = req.into_parts();
+        let replay_body = match ReplayBody::try_new(body, MAX_BUFFERED_BYTES) {


this is really not important at all, but i kind of feel like the prepare_request method should be responsible for checking the size_hint and ReplayBody should just have an unconditional new constructor...that kind of feels like a better separation of concerns to me, with the retry policy code being responsible for determining if the body is too large to retry, and ReplayBody just providing the buffering mechanism.

but, since ReplayBody does still need to be aware of the body size limit so that it can stop buffering if necessary, maybe this is fine...either way, it's not a big deal...

I initially had it the way you suggest, but I think I liked to make it impossible to create a ReplayBody that is 'illegal' -- this means that the redundant check in clone_request becomes totally unnecessary: by virtue of having constructed a ReplayBody, we know that it doesn't violate our expectations on body size.

fair enough, that's a good point!

hawkw · 2021-11-01T17:03:27Z

linkerd/app/core/src/retry.rs

+        let replay_body = match ReplayBody::try_new(body, MAX_BUFFERED_BYTES) {
+            Ok(body) => body,
+            Err(body) => {
+                tracing::debug!(
+                    size = body.size_hint().lower(),
+                    "Body is too large to buffer"
+                );
+                return Either::B(http::Request::from_parts(head, body));
+            }
+        };
+
+        // The body may still be too large to be buffered if the body's length was not known.
+        // `ReplayBody` handles this gracefully.
+        Either::A(http::Request::from_parts(head, replay_body))


nit, take it or leave it: the early return here feels slightly convoluted, can't this just be

Suggested change

let replay_body = match ReplayBody::try_new(body, MAX_BUFFERED_BYTES) {

Ok(body) => body,

Err(body) => {

tracing::debug!(

size = body.size_hint().lower(),

"Body is too large to buffer"

);

return Either::B(http::Request::from_parts(head, body));

}

};

// The body may still be too large to be buffered if the body's length was not known.

// `ReplayBody` handles this gracefully.

Either::A(http::Request::from_parts(head, replay_body))

match ReplayBody::try_new(body, MAX_BUFFERED_BYTES) {

Ok(replay_body) => {

// The body may still be too large to be buffered if the body's length was not known.

// `ReplayBody` handles this gracefully.

Either::A(http::Request::from_parts(head, replay_body))

>,

Err(body) => {

tracing::debug!(

size = body.size_hint().lower(),

"Body is too large to buffer"

);

Either::B(http::Request::from_parts(head, body))

}

}

yeah, I had it that way initially. but it feels like the secondary case is exceptional, so it feels appropriate to early return. don't feel strongly either way, though.

hawkw · 2021-11-01T17:06:40Z

linkerd/http-retry/src/lib.rs

+        if let Some(rest_upper) = rest_hint.upper() {
+            hint.set_upper(buffered + rest_upper);
        }


hmm, if rest doesn't have an upper bound, can't we just use the max buffered length as the upper bound, instead of not returning one?

But we don't know that it's actually the upper limit of the body, do we? My understanding is that ReplayBody should gracefully handle the case when the inner body exceeds the buffer capacity?

ah, yeah, it's only the upper limit if we're a subsequent clone of the body --- if we're the initial body, we will continue reading body chunks if we go past the limit; we'll just stop buffering them. we probably could add a check here and only return the max length if we didn't get an upper bound from rest_hint and we know we're not the original body...but, this is probably fine as-is and it may not be worth bothering with that...

olix0r requested a review from a team October 31, 2021 02:09

olix0r mentioned this pull request Oct 31, 2021

retry: allow retrying requests without Content-Length headers #1341

Merged

olix0r added 2 commits October 31, 2021 02:57

Some random HTTP transparency test failed because we now set content-…

10978d1

…length... but that seems okay

less code is better

2f8bbc5

olix0r commented Oct 31, 2021

View reviewed changes

olix0r added 4 commits October 31, 2021 03:18

+comment

d252370

+debug_assert content-length == size_hint

8dc3fab

touchup ReplayBody::size_hint

f3ecbf5

typo

e065fdf

olix0r marked this pull request as draft October 31, 2021 15:52

olix0r added 3 commits October 31, 2021 16:09

Move length checking into ReplayBody::try_new

d685b31

Move length checking into the ReplayBody constructor

729fc21

olix0r changed the title ~~Simplify retry policy to use Body::size_hint~~ Use Body::size_hint to determine whether a request is retryable Oct 31, 2021

olix0r marked this pull request as ready for review October 31, 2021 16:28

olix0r commented Nov 1, 2021

View reviewed changes

hawkw approved these changes Nov 1, 2021

View reviewed changes

olix0r merged commit dae3917 into eliza/fix-retry-cl Nov 1, 2021

olix0r deleted the ver/can-retry-simplify branch November 1, 2021 17:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Body::size_hint to determine whether a request is retryable#1345

Use Body::size_hint to determine whether a request is retryable#1345
olix0r merged 10 commits intoeliza/fix-retry-clfrom
ver/can-retry-simplify

olix0r commented Oct 31, 2021 •

edited

Loading

Uh oh!

olix0r Oct 31, 2021 •

edited

Loading

Uh oh!

olix0r Nov 1, 2021 •

edited

Loading

Uh oh!

hawkw left a comment

Uh oh!

hawkw Nov 1, 2021

Uh oh!

olix0r Nov 1, 2021

Uh oh!

hawkw Nov 1, 2021

Uh oh!

hawkw Nov 1, 2021

Uh oh!

olix0r Nov 1, 2021

Uh oh!

hawkw Nov 1, 2021

Uh oh!

olix0r Nov 1, 2021

Uh oh!

hawkw Nov 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		let has_body_header = req.headers().contains_key("transfer-encoding")
		\|\| req.headers().contains_key("content-length");

Conversation

olix0r commented Oct 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olix0r Oct 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olix0r Nov 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

olix0r commented Oct 31, 2021 •

edited

Loading

olix0r Oct 31, 2021 •

edited

Loading

olix0r Nov 1, 2021 •

edited

Loading