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

Fix race condition in Connection.serve and AsyncResult.wait #531

Merged
merged 5 commits into from
Mar 18, 2023

Conversation

notEvil
Copy link

@notEvil notEvil commented Mar 15, 2023

#530

The first commit adds the unit test, the second contains the fix. Checkout d861ab9 to confirm the issue using

python -m pytest ./tests/test_race.py

@comrumino comrumino merged commit 99c5abe into tomerfiliba-org:master Mar 18, 2023
@comrumino
Copy link
Collaborator

bcab6a0 has some refactoring that may interest you. by no means final, but another pass.

@notEvil
Copy link
Author

notEvil commented Mar 18, 2023

It completely breaks thread binding, and introduces a busy loop here bcab6a0#diff-506bd4e029401c85a178488d45b05378da956b6f608c64a6e5f9aba74890e8c1R56. I wouldn't change the way thread binding works unless there is a clear gain because there is lots of potential to introduce race conditions (its complexity opens up a lot more windows than default serve). To be fair, the implementation looks bad and overly complicated. There is potential to make it more readable, but the base design is conceptually sound imo. (#491 (comment))

  • replaced _receiving with _recvlock._is_owned as it is slightly more
    robust

how is _receiving not robust?

@comrumino
Copy link
Collaborator

While I broke some things w/ my changes, there were different things were broken prior to it. Test results about latest comment in #530

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.

2 participants