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

fix(http/retry): is_end_stream() is true for empty bodies #3558

Conversation

cratelyn
Copy link
Collaborator

this commit fixes a small, subtle bug in PeekTrailersBody<B>.

if wrapping an empty body, the peek body will inspect the wrong Option<T> in the trailers field with the following type:

    /// The inner body's trailers, if it was terminated by a `TRAILERS` frame
    /// after 0-1 DATA frames, or an error if polling for trailers failed.
    ///
    /// Yes, this is a bit of a complex type, so let's break it down:
    /// - the outer `Option` indicates whether any trailers were received by
    ///   `WithTrailers`; if it's `None`, then we don't *know* if the response
    ///   had trailers, as it is not yet complete.
    /// - the inner `Result` and `Option` are the `Result` and `Option` returned
    ///   by `HttpBody::trailers` on the inner body. If this is `Ok(None)`, then
    ///   the body has terminated without trailers --- it is *known* to not have
    ///   trailers.
    trailers: Option<Result<Option<http::HeaderMap>, B::Error>>,

for an empty body, we know that there are no trailers, which means that we have Some(Ok(None)).

consider also, the documentation of is_end_stream():

An end of stream means that both poll_data and poll_trailers will
return None.

A return value of false does not guarantee that a value will be
returned from poll_stream or poll_trailers.

we can guarantee in this case that poll_trailers() will return Ok(None) since we've already called it and proven that to be the case. we are holding that value, after all.

this change will not affect any behavior w.r.t. what the peek body yields, but it will mean that it reports is_end_stream() correctly when it wraps an empty body.

this commit fixes a small, subtle bug in `PeekTrailersBody<B>`.

if wrapping an empty body, the peek body will inspect the wrong
`Option<T>` in the trailers field with the following type:

```rust
    /// The inner body's trailers, if it was terminated by a `TRAILERS` frame
    /// after 0-1 DATA frames, or an error if polling for trailers failed.
    ///
    /// Yes, this is a bit of a complex type, so let's break it down:
    /// - the outer `Option` indicates whether any trailers were received by
    ///   `WithTrailers`; if it's `None`, then we don't *know* if the response
    ///   had trailers, as it is not yet complete.
    /// - the inner `Result` and `Option` are the `Result` and `Option` returned
    ///   by `HttpBody::trailers` on the inner body. If this is `Ok(None)`, then
    ///   the body has terminated without trailers --- it is *known* to not have
    ///   trailers.
    trailers: Option<Result<Option<http::HeaderMap>, B::Error>>,
```

for an empty body, we *know* that there are no trailers, which means
that we have `Some(Ok(None))`.

consider also, the documentation of `is_end_stream()`:

> An end of stream means that both poll_data and poll_trailers will
> return None.
>
> A return value of false does not guarantee that a value will be
> returned from poll_stream or poll_trailers.

we can guarantee in this case that `poll_trailers()` will return
`Ok(None)` since we've already called it and proven that to be the case.
we *are* holding that value, after all.

this change will not affect any behavior w.r.t. what the peek body
yields, but it will mean that it reports `is_end_stream()` correctly
when it wraps an empty body.

Signed-off-by: katelyn martin <[email protected]>
Copy link
Member

@olix0r olix0r left a comment

Choose a reason for hiding this comment

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

Is this a feat or a fix?

If is_end_stream returned false when it should return true, that wouldn't really be a bug (because is_end_stream false just means we have to poll more). Whereas if it returned true incorrectly, that would be a bug.

I think feat is right, then? But I just wanted to confirm that...

@cratelyn cratelyn changed the title feat(http/retry): is_end_stream() is true for empty bodies fix(http/retry): is_end_stream() is true for empty bodies Jan 22, 2025
@cratelyn
Copy link
Collaborator Author

this commit is definitely a fix, rather than a feature. thank you for calling that out, @olix0r. for other readers: the distinction is important for future calculus about things we would like to backport.

i've ammended the pr title to reflect this!

@cratelyn cratelyn merged commit 907f895 into main Jan 22, 2025
15 checks passed
@cratelyn cratelyn deleted the kate/http-retry.peek-body-observes-empty-end-of-stream-correctly branch January 22, 2025 20:24
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.

2 participants