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

fix(client): Use Stream instead of TrySteam for client calls #61

Merged
merged 9 commits into from
Oct 10, 2019
Merged

fix(client): Use Stream instead of TrySteam for client calls #61

merged 9 commits into from
Oct 10, 2019

Conversation

alce
Copy link
Collaborator

@alce alce commented Oct 8, 2019

Motivation

Currently, client methods take TryStreams as inputs when it's not really necessary. This change makes client calls a bit simpler and potentially helps simplify a solution for #34.

Solution

This PR is a POC that changes input stream bounds from Stream<Item = Result<Message, Status>> to Stream<Item = Message>. As is, client and servers work fine but interop tests need to be adjusted for this PR to be complete.

@alce alce marked this pull request as ready for review October 8, 2019 23:02
@alce
Copy link
Collaborator Author

alce commented Oct 8, 2019

Updating interop was actually very easy. This is ready for review.

@olix0r
Copy link
Contributor

olix0r commented Oct 8, 2019

Currently, client methods take TryStreams as inputs when it's not really necessary.

Can you expand on that? Why isn't it necessary? How does a client get notified of a stream failure (with its status/error message)?

@alce
Copy link
Collaborator Author

alce commented Oct 9, 2019

Hi @olix0r, maybe the title is misleading.

All this PR does is remove the need to wrap outgoing messages in Results for client and bi-directional streams, responses do not change.

So instead of this:

client.method(stream::iter(vec![Ok(1), Ok(2), Ok(3)])).await?;

we can write this

client.method(stream::iter(vec![1, 2, 3])).await?

It is totally possible that I'm missing something but I don't understand the semantics of potentially sending an Err(Status) message as part of a streaming request. If you sneak in an Err(Status...) somewhere in the stream, the server just returns an Unknown error. I don't know what else it could do other than maybe echo the error back.

If the server tries to read all messages in the stream before returning a response, we don't even know how many messages, (or which ones) it managed to process.

@olix0r
Copy link
Contributor

olix0r commented Oct 9, 2019

Ah! I see: this is for request streams and not response streams. That makes sense, then.

@LucioFranco
Copy link
Member

I think we might want to update this function to not take a result? https://github.com/hyperium/tonic/blob/master/tonic/src/codec/encode.rs#L32

@alce
Copy link
Collaborator Author

alce commented Oct 9, 2019

@LucioFranco
Copy link
Member

@alce I must be tired I totally missed that! 😅

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Thanks for doing this <3

@LucioFranco LucioFranco changed the title Use Stream instead of TrySteam for client calls fix(client): Use Stream instead of TrySteam for client calls Oct 10, 2019
@LucioFranco LucioFranco merged commit 7eda823 into hyperium:master Oct 10, 2019
@alce alce deleted the input-streams branch October 10, 2019 16:14
blittable pushed a commit to blittable/tonic that referenced this pull request Oct 22, 2019
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.

3 participants