feat(body): add trailers to Body channel (#2260) #2387
Conversation
|
@seanmonstar Should I also add "try_send_trailers" to the Sender? |
|
Thanks for the PR!
I don't think that's necessary, unless you have a good reason for it? |
|
|
@seanmonstar I ran I ran each one multiple times to make sure that everything works fine and I saw that adding trailers didn't cause much of a difference. And here are some more results: Faster execution of the version with trailers -> https://pasteboard.co/JJfuW5J.png |
src/body/body.rs
Outdated
| .try_send(Ok(chunk)) | ||
| .map_err(|_| crate::Error::new_closed()) | ||
| } | ||
|
|
||
| /// Send trailers on trailers channel. | ||
| pub fn send_trailers(&mut self, trailers: HeaderMap) -> crate::Result<()> { |
There was a problem hiding this comment.
Even though not specifically required due to the internal implementation, I'm thinking this should be an async fn, so that we could adjust the implementation later if need be. But I'm open to hearing why it shouldn't.
| @@ -382,7 +382,7 @@ impl HttpBody for Body { | |||
| .. | |||
| } => match ready!(Pin::new(trailers_rx).poll(cx)) { | |||
| Ok(t) => Poll::Ready(Ok(Some(t))), | |||
| Err(_) => Poll::Ready(Err(crate::Error::new_closed())), | |||
| Err(_) => Poll::Ready(Ok(None)), | |||
There was a problem hiding this comment.
@seanmonstar If we don't send any trailers, polling the oneshot receiver returns Err(Canceled) so if we return Err(..) in this case, receive process will result in error. That's why I changed this to Ok(None). This way we don't have to send empty trailers. Is it ok though?
There was a problem hiding this comment.
Oh yes, this change is the correct behavior. Otherwise, when people upgrade hyper then suddenly in H2 when the trailers are polled they'd error the stream since they weren't sending any before.
Closes #2260.