storj / drpc

drpc is a lightweight, drop-in replacement for gRPC
MIT License
1.49k stars 50 forks source link

Detect end of stream on `drpcconn.Conn` #11

Closed maxtruxa closed 3 years ago

maxtruxa commented 3 years ago

Right now no API exists to detect when an instance of drpcconn.Conn got disconnected without polling in some form. I could identify two ways to learn about a disconnect:

Something like a WaitForClosed() method which blocks until the connection is closed or - even better - a channel to wait on would be really useful. The connection's Closed() just calls the connection manager's Closed(), which in turn checks the term signal. That signal appears to be exactly what I'm looking for.

I'd like to create a PR to resolve this issue. Would it be acceptable to (indirectly) expose the term signal's Wait() and/or Signal() methods on drpcmanager.Manager and drpconn.Conn (e.g. as WaitForClosed()/ClosedSignal())?

My use case: I have long-running DRPC connections, on which requests are sent every so often. Most of the time the client just idles, waiting for an external event to occur. This idling process should be stopped as soon as the DRPC connection goes away.

zeebo commented 3 years ago

I think this makes sense. Currently, the Closed call is used by a connection pooling implementation so that it can be polled once before it is returned to attempt to catch as many connections that died in the pool as possible. Having it return a <-chan struct{} would be more general, easily done, and probably cover many use cases.

I don't know if it's worth it to make a breaking change for that, though. I'm leaning towards yes because it's in the drpc.Conn interface, and it's still early in the library's lifetime.

Also, I would be elated if you made a PR for the change. I'll try to get to it sometime in the next week or so.

maxtruxa commented 3 years ago

For the first draft I went with the breaking change, mainly because I couldn't settle on a good name for the method (ClosedChan, ClosedSignal, Done, ...).

zeebo commented 3 years ago

I believe this has been fixed by #16. Thanks!