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 thread safety reentry #203

Merged
merged 3 commits into from
Aug 30, 2022

Conversation

phodge
Copy link
Contributor

@phodge phodge commented Apr 22, 2022

Fixes broken tests described in #201 by switching to an RLock. Also added a bunch of comments to explain what's going on.

@phodge
Copy link
Contributor Author

phodge commented Apr 22, 2022

@jamielennox: Unfortunately I haven't been able to find a better solution than RLock, but have added some extra comments to try and make life easier for whoever has to maintain this in the future. I added a timeout of 10 seconds for the thread locking to try and prevent tests blocking indefinitely, which might be a small quality of life improvement but it ended up being a bit verbose trying to support python2. If you think it's too much I can rip it all out and go back to just with _send_lock:.

@phodge
Copy link
Contributor Author

phodge commented May 17, 2022

Hey @jamielennox, just wanted to check in again and see if there's anything else I can do to help get this merged and released?

@jamielennox jamielennox merged commit 9264284 into jamielennox:master Aug 30, 2022
@jamielennox
Copy link
Owner

I'm sorry this has been so long. Admittedly i'm a little scared of this one, i've no idea what edge cases it might trigger but we can release it and see.

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