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

Added in a close() method for the nsq Reader. It closes all connections ... #100

Merged
merged 1 commit into from
Oct 13, 2014
Merged

Conversation

jonesetc
Copy link
Contributor

@jonesetc jonesetc commented Oct 8, 2014

...and stops all PeriodicCallbacks. To close those callbacks, the _run() method needed to be updated so that we could keep track of them.

…ns and stops all PeriodicCallbacks. To close those callbacks, the _run() method needed to be updated so that we could keep track of them.
@jonesetc
Copy link
Contributor Author

jonesetc commented Oct 8, 2014

This is a first stab at a close functionality for the Reader. I expect that there are some gaps in steps needed.

Relevant to, but not the full solution to #70

@mreiferson
Copy link
Member

@jonesetc this looks good so far, thanks for the contribution!

Do you plan on continuing to add the rest of the missing pieces for #70?

@jonesetc
Copy link
Contributor Author

I was only planning on adding this to the reader. It's an itch I needed to scratch for a work project.

However, if this is merged in I can take a look as to what goes into the rest of #70 this weekend. Then I'll give it a shot and open a new pull request for it.

@mreiferson
Copy link
Member

OK, fair enough - would be great to finish up the rest of #70 if you're interested!

This LGTM

mreiferson added a commit that referenced this pull request Oct 13, 2014
Added in a close() method for the nsq Reader. It closes all connections ...
@mreiferson mreiferson merged commit 609150d into nsqio:master Oct 13, 2014
@mreiferson
Copy link
Member

Thanks ✨

@crossjam crossjam mentioned this pull request Oct 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants