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

examples: Replace context.Background with context.WithTimeout #1877

Merged
merged 4 commits into from
Feb 28, 2018

Conversation

menghanl
Copy link
Contributor

@menghanl menghanl commented Feb 20, 2018

example clean up after #1854

@vitalyisaev2
Copy link
Contributor

I would just like to mention that for streams simple cancellation (context.WithCancel instead of context.WithTimeout) could be a little bit more idiomatic. Steams may be potentially infinite, so timeouting on working stream may cause data inconsistency and so on.

@menghanl
Copy link
Contributor Author

@vitalyisaev2 Thanks for taking a look!
That's a good point. I have updated the code accordingly. PTAL.

Copy link
Contributor

@vitalyisaev2 vitalyisaev2 left a comment

Choose a reason for hiding this comment

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

This patch is fully in line with streaming protocol changes proposed in #1854, LGTM.

@menghanl menghanl assigned menghanl and MakMukhi and unassigned menghanl Feb 22, 2018
@dfawley
Copy link
Member

dfawley commented Feb 27, 2018

I would just like to mention that for streams simple cancellation (context.WithCancel instead of
context.WithTimeout) could be a little bit more idiomatic. Steams may be potentially infinite, so
timeouting on working stream may cause data inconsistency and so on.

I disagree; even streaming RPCs should always have deadlines set. One problem that could occur without a deadline is that if the RPC is wait-for-ready, it will block indefinitely if all servers are down. Since your stream can't be expected to live forever (due to inevitable connection errors), if you want it to be persistent, you'll need to have code to recreate it when it fails. This same code can recreate it when the deadline is reached as well.

@menghanl menghanl requested review from dfawley and removed request for MakMukhi February 27, 2018 21:13
@menghanl menghanl assigned dfawley and unassigned MakMukhi Feb 27, 2018
@vitalyisaev2
Copy link
Contributor

@dfawley Unfortunately I'm not aware of 'wait-for-ready` RPC semantics... But I think we can consider simple and wide-spread examples of streaming.

For instance, suggest we've implemented file upload / download protocol on the top of the GRPC stream (so it would be client-side streaming for uploading and server-side streaming for downloading). We have to download a huge 10GB file, but we have weak connection with low throughput. If we set a timeout for this kind of stream, we'll never be able to download the file, because the stream will be canceled and next time download will start from the very beginning.

Another example is a gossip protocol utilizing GRPC bidirectional streaming. From my point of view, this stream should not be terminated by timeout either. Because data exchange between peers should take place during the whole lifetime of peer processes. I agree with you that reconnection and retrial logic should be implemented in any case, but I don't see a good reason to terminate the stream which is actually expected to live forever.

@dfawley dfawley changed the title exmaples: Replace context.Background with context.WithTimeout examples: Replace context.Background with context.WithTimeout Feb 27, 2018
@dfawley
Copy link
Member

dfawley commented Feb 27, 2018

@vitalyisaev2,

Unfortunately I'm not aware of 'wait-for-ready` RPC semantics

There are other ways an RPC can get stuck without a deadline. If the resolver never resolves the target name when the client is created, even fail-fast (non-wait-for-ready) RPCs will block indefinitely. In general, it's a bad idea to perform unbounded, blocking operations. If you have other things in place to prevent it from blocking forever (like SIGINTing the whole process), then I guess it should be OK. But I'd prefer for our "official" examples of streaming RPCs to use a timeout.

the stream will be canceled and next time download will start from the very beginning.

This may be a problem with the API design; if you're transmitting large files over an unreliable network, you should ideally be able to resume them from the middle after reconnecting.

@dfawley dfawley merged commit f0a1202 into grpc:master Feb 28, 2018
@menghanl menghanl added the Type: Documentation Documentation or examples label Mar 1, 2018
@menghanl menghanl added this to the 1.11 Release milestone Mar 1, 2018
@menghanl menghanl deleted the no_background_ctx branch April 5, 2018 22:19
@lock lock bot locked as resolved and limited conversation to collaborators Oct 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Documentation Documentation or examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants