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

pubsub: discussion regarding use of context.Context to cancel Receive #1304

Open
nhooyr opened this Issue Feb 5, 2019 · 5 comments

Comments

Projects
None yet
3 participants
@nhooyr
Copy link

nhooyr commented Feb 5, 2019

https://godoc.org/cloud.google.com/go/pubsub#Subscription.Receive

Using the context to cancel the Receive feels like abuse of context.Context. A pubsub receive is very often a long running operation and something that will be explicitly shutdown during the shutdown of a server. It's not something where cancellation needs to transit multiple APIs and so using context.Context as the main way to handle cancellation here seems awkward.

I'm not sure if anything can be done about this now but I wanted to see what other people thought about it. As for a better API, I'm not entirely sure but a method somewhere that cancels the receive and waits for it to return, just like *http.Server.Close().

@nhooyr nhooyr changed the title pubsub: use of context.Context to cancel Receive pubsub: discussion regarding use of context.Context to cancel Receive Feb 5, 2019

@jadekler

This comment has been minimized.

Copy link
Collaborator

jadekler commented Feb 6, 2019

A receive is something that will be explicitly shutdown during the shutdown of the server

I don't know what you mean by explicitly. Could you elaborate? There's no code that makes Receive end when a server is shutting down, unless the user writes it.

It's not something where cancellation needs to transit multiple APIs

I don't know what you mean by API, but the cancellation does affect several stacks including our library, the gRPC library, and the network to the service (since it encompasses a long-running RPC).

using context.Context as the main way to handle cancellation here seems awkward

I'm not sure I can agree with that. The context documentation states it "[...] carries deadlines, cancelation signals, and other request-scoped values across API boundaries and between processes" and that "Calling the CancelFunc cancels the child and its children, removes the parent's reference to the child, and stops any associated timers.". I think our usage is an idiomatic usage of context and cancellation. I might be misunderstanding your discussion topic, though? Perhaps you could elaborate on what you're thinking of for other, less awkward ways to handle cancellation?

@jadekler jadekler self-assigned this Feb 6, 2019

@nhooyr

This comment has been minimized.

Copy link
Author

nhooyr commented Feb 17, 2019

@jadekler a more clear way to phrase my question would be:

// initialize l as a net.Listener
// initialize s as *grpc.Server
// Serve returns when the context is cancelled.
err := s.Serve(ctx, l)
// ... handle err

Do you believe that would be a better API for *grpc.Server versus the explicit Stop method we have now?

@nhooyr

This comment has been minimized.

Copy link
Author

nhooyr commented Feb 17, 2019

I don't know what you mean by explicitly. Could you elaborate? There's no code that makes Receive end when a server is shutting down, unless the user writes it.

You know how you create a grpc.Server, then call s.Serve to start listening? You call s.Stopto shutdown the server. Thats what I mean by explicit cancellation versus cancelling a context.

I don't know what you mean by API, but the cancellation does affect several stacks including our library, the gRPC library, and the network to the service (since it encompasses a long-running RPC).

The library can have an internal context for that, it doesn't need a user provided context.

I'm not sure I can agree with that. The context documentation states it "[...] carries deadlines, cancelation signals, and other request-scoped values across API boundaries and between processes" and that "Calling the CancelFunc cancels the child and its children, removes the parent's reference to the child, and stops any associated timers.". I think our usage is an idiomatic usage of context and cancellation. I might be misunderstanding your discussion topic, though? Perhaps you could elaborate on what you're thinking of for other, less awkward ways to handle cancellation?

In general, the way I've seen context used is that if there is a API that doesn't return a result, but is instead a long lived service, you should explicitly shut it down versus controlling shutdown via a context as it becomes easier to properly handle shutdown.

E.g. right now, if I wanted to start graceful shutdown of a server that receives pubsub message, I'd have to cancel the pubsub context and then wait for the pubsub receive goroutine to finish. In effect, I'd have to add my own synchronization. Whereas, If Receive() instead returned a structure on which I could call a Serve method to start serving and then later a Shutdown method to stop , I just have to call Shutdown and it would wait until everything is done. It can use context's internally to accomplish that, just shouldn't be the exposed API to a user.

@jba

This comment has been minimized.

Copy link
Contributor

jba commented Feb 17, 2019

There's not much difference here. Either Receive is synchronous and shutting it down isn't, as we have now, or Receive is asynchronous and Shutdown isn't, as with your Serve and Shutdown. Since most uses of Receive don't even need to shut down, we thought it was better to make is synchronous. Note that you see ListenAndServe (synchronous) more often than Serve (async).

@nhooyr

This comment has been minimized.

Copy link
Author

nhooyr commented Feb 17, 2019

Since most uses of Receive don't even need to shut down, we thought it was better to make is synchronous.

I can't see why most use cases of receive wouldn't need to shutdown. Wouldn't you want to ensure all your message handlers are done before exiting?

Note that you see ListenAndServe (synchronous) more often than Serve (async).

Only in examples. In production, you'd want to gracefully shutdown in which case ListenAndServe becomes inadequate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment