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

Conversation

yungchin
Copy link
Contributor

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.

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>
The parent commit dropped the use of Event() because it seemed
unnecessary, but of course namedtuples are immutable, which meant
invisible exceptions in the finaliser, so let's just restore the use of
Event(), this being the quickest fix.

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

Not to be too much of a codecov cheerleader, but their patch-coverage feature just allowed me to spot a bug that was hidden from the test suite: https://codecov.io/github/Parsely/pykafka/compare/f0f12a3...2d4a35a showed that there's only partial branch coverage for the while not shared.ending: line, which was due to me introducing a bug in the finaliser (and exceptions in finalisers are suppressed, so it went otherwise unnoticed).

@stevepeak

@stevepeak
Copy link

👍 Thank you @yungchin Very happy to provide this solution for you!

@emmettbutler
Copy link
Contributor

@yungchin this looks good. I'm pretty happy with any fix that removes a weakref, as those generally make the code harder to reason about. I spent a bit trying to come up with a better name for shared that's more descriptive of what it's doing, but I can't.

emmettbutler added a commit that referenced this pull request Oct 21, 2015
handlers: fix RequestHandler queue flush on stop
@emmettbutler emmettbutler merged commit c72eea4 into master Oct 21, 2015
@emmettbutler emmettbutler deleted the bugfix/reqhandler-race-#309 branch October 21, 2015 18:37
@yungchin
Copy link
Contributor Author

@emmett9001 thanks as ever for the speediness!! I actually think that shared is the worst name possible, but still couldn't think of a better one. It's like they say http://martinfowler.com/bliki/TwoHardThings.html

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.

race condition in RequestHandler?
4 participants