feat(response): introduce trailers support#2788
Conversation
|
After seeing your implementation, I just realized my implementation#2782 which mimics the fn chunk may consumes the res body and lead to inaccessibility of the body after the fn trailers called. Though, I think the user cases are mostly getting trailers after getting the body. Did not expect to be this complicate for this issue. |
anuraaga
left a comment
anuraaga
left a comment
There was a problem hiding this comment.
Hi @0x676e67 - I am also interested in trailers support and find this PR. It looks pretty good to me, requiring the body to be consumed to have access to trailers is sensible and is how Go's stdlib http client works too.
I left one suggestion but does it seem worth giving the PR a go?
| /// be determined after processing the entire response body. | ||
| #[inline] | ||
| pub async fn trailers(&mut self) -> crate::Result<Option<HeaderMap>> { | ||
| match self.trailers_rx.try_recv() { |
There was a problem hiding this comment.
Because this channel isn't actually used for synchronization, does it make sense to use a simpler OnceLock instead? Then trailers can be returned as a reference to match headers and doesn't need to be async.
There was a problem hiding this comment.
Feel free to open a PR, I might not have the time to implement it myself.
0x676e67
force-pushed
the
trailers
branch
2 times, most recently
from
ea08f51 to
599a5b9
Compare
| // Boxed to save space (11 words to 1 word), and it's not accessed | ||
| // frequently internally. | ||
| url: Box<Url>, | ||
| trailers: Option<HeaderMap>, |
There was a problem hiding this comment.
For something that I assume is not used often by most people, I'd rather sacrifice a little bit of perf for the less used case, instead of increasing the size for everyone. This could probably be stored with a private extension in res.extensions(). What do you think?
There was a problem hiding this comment.
I completely agree with that — I’ll do it that way later.
There was a problem hiding this comment.
This has already been done. Also, should we consider extending ResponseBuilderExt to support trailers, so that trailers can be preserved when converting from http::Response to reqwest::Response?
There was a problem hiding this comment.
That I'm not so sure about... I think of this as more a temporary storage location. If a user converts to an http::Response, then there's the normal lower level way to get trailers, no?
There was a problem hiding this comment.
That makes sense. We can leave this out for now.
0x676e67
force-pushed
the
trailers
branch
2 times, most recently
from
ae23d64 to
c2c9132
Compare
There was a problem hiding this comment.
Thanks for the help on this @0x676e67! I was able to integrate it easily into a python wrapper around reqwest - I feel the current API is just fine
I'll be able to move forward implementing grpc client support in connect-python thanks to it.
4 participants
from: #2626
from: #2776
Just a draft prototype for now. It’s a bit ugly, and there’s probably a better approach