Skip to content

Commit f049ca8

Browse files
committed
feat(http/retry): is_end_stream() is true for empty bodies
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 <kate@buoyant.io>
1 parent 399ab1d commit f049ca8

File tree

1 file changed

+13
-3
lines changed

1 file changed

+13
-3
lines changed

linkerd/http/retry/src/peek_trailers.rs

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,18 @@ where
191191

192192
#[inline]
193193
fn is_end_stream(&self) -> bool {
194-
self.first_data.is_none() && self.trailers.is_none() && self.inner.is_end_stream()
194+
let Self {
195+
inner,
196+
first_data,
197+
trailers,
198+
} = self;
199+
200+
let trailers_finished = match trailers {
201+
Some(Ok(Some(_)) | Err(_)) => false,
202+
None | Some(Ok(None)) => true,
203+
};
204+
205+
first_data.is_none() && trailers_finished && inner.is_end_stream()
195206
}
196207

197208
#[inline]
@@ -254,8 +265,7 @@ mod tests {
254265
let empty = MockBody::default();
255266
let peek = PeekTrailersBody::read_body(empty).await;
256267
assert!(peek.peek_trailers().is_none());
257-
// TODO(kate): this will not return `true`.
258-
// assert!(peek.is_end_stream());
268+
assert!(peek.is_end_stream());
259269
}
260270

261271
#[tokio::test]

0 commit comments

Comments
 (0)