PYTHON-644 - Timer logic#690
Merged
Merged
Conversation
This reverts commit 1a01f11 for PYTHON-644. The "create timer on request" strategy can let timers leak.
This allows us to revert some of the fix to PYTHON-367 without the timer heap growing out of control as described on that ticket. We cancel before defuncting to ensure that the timer doesn't hang around while running callbacks. Between this change and the delegation of callbacks to a secondary thread (see commit 5192302, part of the original PYTHON-367 fix), this commit should not cause a regression on PYTHON-367. Note that we don't cancel timers before defuncting and retrying -- we're not done with the logical request and still want that timer to fire. Now that timers are started on ResponseFuture initialization, we actually use the value of timeout in the tests, so we no longer pass that argument, allowing the test to use the default value.
Contributor
Author
|
Travis tests on my fork all passed, except the expected Python 2.6 errors. |
Contributor
|
LGTM Jim! I tested and confirm it fixes the issue. (Tested with libev and asyncore) |
beltran
reviewed
Jan 25, 2017
| # we got some other kind of response message | ||
| msg = "Got unexpected message: %r" % (response,) | ||
| exc = ConnectionException(msg, host) | ||
| self._cancel_timer() |
Contributor
There was a problem hiding this comment.
I was wondering if this line is really necessary, as the timer will be canceled inside inside
self._set_final_exception(exc)
Contributor
Author
There was a problem hiding this comment.
The goal is to cancel as quickly as possible -- without waiting for callbacks to execute in defunct below -- to avoid a pileup of timers when clients don't back off requests on a failing connection as described in PYTHON-367. Do you think this accomplishes that goal?
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First commit reverts part of the fix for PYTHON-367 (#379) that put
_start_timercalls in code that could be called from secondary threads. This caused a race between timer cancellation and that_start_timercall.The second commit adds a
_cancel_timercall before defuncting connections if theResponseFutureisn't going to retry the request. Commit message explaining this reproduced below: