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

fixes #449 #455

Merged
merged 1 commit into from
Sep 4, 2021
Merged

fixes #449 #455

merged 1 commit into from
Sep 4, 2021

Conversation

wumb0
Copy link
Contributor

@wumb0 wumb0 commented Aug 26, 2021

I was experiencing hangs similar to #449, #437, and #354.
The issue I was seeing is basically a race condition present in the AsyncResult class.
If another thread happens to get the request data in a call to Connection.serve then AsyncResult.wait will end up going around the while not self._is_ready loop an additional time, which is why there is a timeout on the request, but no "result expired" exception. The other thread will call the thread callback (Connection._seq_request_callback), but may not set _is_ready in time for the loop in AsyncResult.wait to break without re-entering Connection.serve.

This PR does fix the hanging issues for my application, and I suspect it will help others as well, but I do not think it addresses the root of the problem: that other threads may call callbacks on async requests at the Connection level.

This fix changes the AsyncResult._is_ready variable from a bool to a threading.Event and waits on it if the call to Connection.serve in AsyncResult.wait returns True (data was dispatched).

Let me know if you have questions, comments, revisions, etc.

Hope this helps!

@comrumino comrumino self-assigned this Aug 27, 2021
@comrumino comrumino added the Triage Investigation by a maintainer has started label Aug 27, 2021
@comrumino comrumino merged commit 8518e3b into tomerfiliba-org:master Sep 4, 2021
@comrumino comrumino added Done The issue discussion is exhausted and is closed w/ comment and removed Triage Investigation by a maintainer has started labels Sep 4, 2021
@wumb0
Copy link
Contributor Author

wumb0 commented Nov 12, 2021

This code was undone by #463 so #449 needs to be re-opened.
Edit: Disregard, the PR was confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Done The issue discussion is exhausted and is closed w/ comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants