Skip to content
This repository was archived by the owner on Mar 24, 2021. It is now read-only.

Conversation

yungchin
Copy link
Contributor

@yungchin yungchin commented Sep 7, 2015

This fixes #257 by making sure RequestHandler instances are gc'able.

Because it removes the atexit flow completely, it also indirectly deals with #218 - with the changes here, RequestHandler.stop will run later in the teardown process, so that the consumers are already gone, so they no longer keep pressure on the request queue.

This fixes a problem where, because RequestHandler instances registered
themselves with atexit, they would always live until program exit.

Having replaced the atexit call with the more usual __del__ finaliser, I
found that this __del__ would never run: the queue-handling thread
instantiated by RequestHandler holds references to self.  Having also
replaced that ref by a weakref, we did get the finaliser to run.

Finally, to make sure we don't block on Queue.get() forever (in which
case the thread still doesn't die), this also adds a timeout to that.

Signed-off-by: Yung-Chin Oei <yungchin@yungchin.nl>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have some warning message here - perhaps just at loglevel INFO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks yes, coming up!

Small addition to the parent commit.  I figured we might also benefit
from a log line in RequestHandler.stop, since it took a while to realise
that might hang (see #218).

Signed-off-by: Yung-Chin Oei <yungchin@yungchin.nl>
@emmettbutler
Copy link
Contributor

This change is kind of scary in terms of how much it touches, but I've done a bunch of testing of both the consumer and producer on this branch, and I can't find any broken behavior. I think it's important to get atexit out of this module, since it really doesn't make sense to assume that any RequestHandler will live until the interpreter itself dies. This pull request doesn't cause problems when you produce directly from a newly-instantiated KafkaClient and save no references to it, since the Producer worker threads hold their own references to the brokers. I believe I've convinced myself that this PR isn't dangerous.

@emmettbutler emmettbutler self-assigned this Sep 19, 2015
emmettbutler added a commit that referenced this pull request Sep 19, 2015
Get rid of zombie RequestHandler threads
@emmettbutler emmettbutler merged commit a93e69f into master Sep 19, 2015
@emmettbutler emmettbutler deleted the fix/requesthandler-zombies branch September 19, 2015 17:09
yungchin added a commit that referenced this pull request Oct 21, 2015
This resolves #309 by removing the weakref solution introduced in #258,
which would potentially throw a ReferenceError before the worker was
quite done emptying the request queue.

Signed-off-by: Yung-Chin Oei <yungchin@yungchin.nl>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

forget to disconect socket ?
3 participants