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

Detect end of stream on drpcconn.Conn #11

Closed
maxtruxa opened this issue May 25, 2021 · 3 comments
Closed

Detect end of stream on drpcconn.Conn #11

maxtruxa opened this issue May 25, 2021 · 3 comments

Comments

@maxtruxa
Copy link
Contributor

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:

  • The next attempt to send something on the connection fails with "closed: end of stream".
  • There is Closed() on drpcconn.Conn which exposes the desired information, but has to be polled.

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
Copy link
Collaborator

zeebo commented Jun 9, 2021

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
Copy link
Contributor Author

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
Copy link
Collaborator

zeebo commented Jun 16, 2021

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

@zeebo zeebo closed this as completed Jun 16, 2021
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 a pull request may close this issue.

2 participants