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

feat(body): add trailers to Body channel (#2260) #2387

Merged
merged 8 commits into from
Jan 15, 2021
Merged

feat(body): add trailers to Body channel (#2260) #2387

merged 8 commits into from
Jan 15, 2021

Conversation

aeryz
Copy link
Contributor

@aeryz aeryz commented Jan 6, 2021

Closes #2260.

@aeryz aeryz marked this pull request as draft January 7, 2021 17:12
@aeryz
Copy link
Contributor Author

aeryz commented Jan 7, 2021

@seanmonstar Should I also add "try_send_trailers" to the Sender?

@seanmonstar
Copy link
Member

Thanks for the PR!

Should I also add "try_send_trailers" to the Sender?

I don't think that's necessary, unless you have a good reason for it?

@aeryz
Copy link
Contributor Author

aeryz commented Jan 8, 2021

Thanks for the PR!

Should I also add "try_send_trailers" to the Sender?

I don't think that's necessary, unless you have a good reason for it?

try_send_data is implemented for threads that doesn't have an async context. By using try_send_trailers, they can send trailers as well.

@aeryz aeryz marked this pull request as ready for review January 10, 2021 13:47
@aeryz
Copy link
Contributor Author

aeryz commented Jan 12, 2021

@seanmonstar I ran end_to_end benchmarks.
No trailers -> https://pasteboard.co/JJfrwqV.png
Trailers -> https://pasteboard.co/JJfsdsf.png

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
Faster execution of the version without trailers -> https://pasteboard.co/JJfvI6D.png

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Alright, I'm fine with the performance implications (doesn't seem to be any). I've left a few final thoughts inline, but in general I'm happy with how this turned out, thanks!

benches/end_to_end.rs Outdated Show resolved Hide resolved
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<()> {
Copy link
Member

Choose a reason for hiding this comment

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

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.

src/client/conn.rs Outdated Show resolved Hide resolved
@@ -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)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me.

@seanmonstar seanmonstar merged commit bf8d74a into hyperium:master Jan 15, 2021
@aeryz aeryz deleted the body-chan-trailers branch January 16, 2021 14:31
BenxiangGe pushed a commit to BenxiangGe/hyper that referenced this pull request Jul 26, 2021
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.

Add sending trailers on Body channel
2 participants