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

Clean up EncodeBody API #1924

Merged
merged 7 commits into from
Sep 26, 2024
Merged

Clean up EncodeBody API #1924

merged 7 commits into from
Sep 26, 2024

Conversation

djc
Copy link
Contributor

@djc djc commented Sep 5, 2024

Follow-up to #1884 to simplify the newly public low-level EncodeBody API.

cc @zakhenry

@djc djc requested a review from tottoto September 5, 2024 11:33
@zakhenry
Copy link
Contributor

zakhenry commented Sep 5, 2024

Nice one, I'm trying out this branch with my project and are getting the following error in a grpc call that uses Role::Client.

Received RST_STREAM with code 0 (Call ended without gRPC status)

Working through the issue to see if I can find where I might have gone wrong

@zakhenry
Copy link
Contributor

zakhenry commented Sep 5, 2024

Whoops never mind my bad; I accidentally picked the wrong role (should have been Server). So yep confirmed this works for both client and server (my application uses both).

Copy link
Collaborator

@tottoto tottoto left a comment

Choose a reason for hiding this comment

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

Looks good to me. I left some comments.

tonic/src/codec/encode.rs Outdated Show resolved Hide resolved
tonic/src/codec/encode.rs Outdated Show resolved Hide resolved
tonic/src/codec/encode.rs Outdated Show resolved Hide resolved
@djc djc changed the title Encode body api Clean up EncodeBody API Sep 7, 2024
@zakhenry
Copy link
Contributor

@tottoto are there any outstanding blockers to this going in? Hoping for it to be in the next release so I can stop depending on a branch

@tottoto
Copy link
Collaborator

tottoto commented Sep 19, 2024

I was thinking again about the API to be made public, and considering that Side (Role) is used simply to switch constructors, it seems troublesome for users to specify Side when constructing it. Since tokio-stream exposes Fuse type, the constructor type has become simpler, so I felt that new_{server,client} would be sufficiently simple.

Alternatively, it seems that using EncodeBody for both the client and the server is what is causing the complexity, so one option would be to separate it into one for the client and one for the server, and have each focus on a simpler role. However, this would be a relatively big change, so it does not seem to need to consider it for now.

@zakhenry
Copy link
Contributor

As someone that use both the original api and this new one, I found no issues with the new one; it made perfect sense to need to pass a single enum variant to indicate what direction the encoding was happening.

Worth pointing out too that this api is really only for advanced users writing complicated middleware that need to peek/modify the inbound/outbound message bodies so imo the ergonomics of this one call is far overshadowed by the complexity of having to deal with creating a new tower layer.

@LucioFranco LucioFranco self-requested a review September 20, 2024 13:22
tonic/src/client/grpc.rs Outdated Show resolved Hide resolved
U: Stream<Item = Result<T::Item, Status>>,
{
// `source` should be fused stream.
impl<T: Encoder, U: Stream> EncodedBytes<T, U> {
Copy link
Member

Choose a reason for hiding this comment

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

nit: I still prefer the where style when there is 1+ generic, can be done in follow up.

@LucioFranco LucioFranco added this pull request to the merge queue Sep 26, 2024
Merged via the queue into master with commit 3c900eb Sep 26, 2024
17 checks passed
@LucioFranco LucioFranco deleted the encode-body-api branch September 26, 2024 15:45
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.

4 participants