Skip to content

Added thread safety to Zeroconf.close()#41

Closed
justingiorgi wants to merge 2 commits into
python-zeroconf:masterfrom
justingiorgi:master
Closed

Added thread safety to Zeroconf.close()#41
justingiorgi wants to merge 2 commits into
python-zeroconf:masterfrom
justingiorgi:master

Conversation

@justingiorgi
Copy link
Copy Markdown

Zeroconf.close() immediately closed sockets which child threads may
still need to finish their operations. This was causing EBADF (bad file
descriptor) errors in some cases. Zeroconf.close() now blocks until all
known child threads have stopped then closes the sockets safely. If a
child thread was created that Zeroconf is not aware of (ie a
ServiceBrowser object manually created but not registered in
zeroconf.browsers) the EBADF errors may still occur.

Also some of the unit tests were not in a unittest.TestCase subclass so
they were not running. Added test cases to Misc TestCase and added
default execution of unittest.main().

Zeroconf.close() immediately closed sockets which child threads may
still need to finish their operations. This was causing EBADF (bad file
descriptor) errors in some cases. Zeroconf.close() now blocks until all
known child threads have stopped then closes the sockets safely. If a
child thread was created that Zeroconf is not aware of (ie a
ServiceBrowser object manually created but not registered in
zeroconf.browsers) the EBADF errors may still occur.

Also some of the unit tests were not in a unittest.TestCase subclass so
they were not running. Added test cases to Misc TestCase and added
default execution of unittest.main().
@jstasiak
Copy link
Copy Markdown
Collaborator

Hey @justingiorgi, thank you for the contribution. One correction - all tests are running just fine, proof: https://travis-ci.org/jstasiak/python-zeroconf/jobs/94120747 (nose doesn't require tests to be defined in a class or for that class to inherit from TestCase)

With that in mind can you commit again just the changes related to the issue you experienced?

@justingiorgi
Copy link
Copy Markdown
Author

Hey Jakub, that makes sense. Sorry I'm not familiar with nose. I've removed
the changes to the tests.

On Thu, Mar 10, 2016 at 2:11 PM Jakub Stasiak notifications@github.com
wrote:

Hey @justingiorgi https://github.com/justingiorgi, thank you for the
contribution. One correction - all tests are running just fine, proof:
https://travis-ci.org/jstasiak/python-zeroconf/jobs/94120747 (nose
doesn't require tests to be defined in a class or for that class to inherit
from TestCase)

With that in mind can you commit again just the changes related to the
issue you experienced?


Reply to this email directly or view it on GitHub
#41 (comment)
.

@stephenrauch
Copy link
Copy Markdown
Collaborator

Thanks so much for the pull request.

I made several improvements to ZC() shutdown, including incorporating these changes, before I had access to this repo. As of now I believe the functionality from this PR has been incorporated, and I unfortunately have no good way to merge the PR to give you credit for the change, so I am going to close it.

If you think this an error, or an otherwise bad idea, please comment back.

Thanks again for your help.

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 this pull request may close these issues.

3 participants