-
-
Notifications
You must be signed in to change notification settings - Fork 29.2k
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
del _limbo[self] KeyError #72095
Comments
To reproduce:
Chance to reproduce ~20% in my setup. Instead, I get:
|
Successfully reproduced on 2.7.12 and 3.5.2 . Attached patch makes it so limbo, our starting threads' waiting room, is checked first. |
@rooter, if you go this way, you should also |
@Dima.Tisnek, only reason for having both of these conditions together is so I won't have to repeat the same error message in the code at little price of the performance in this edge case (trying to start the same thread multiple times). Unless I'm missing something there should be no way how it would make this lock required for setting |
Your logic is accurate; _started is in fact separate from _limbo. As such taking a lock for "test-then-set" only would suffice. Now when you bring the other primitive under this lock in one place, it would look cleaner if it was also brought in the other. There's one more issue with proposed change: Before the change, if "already started" exception is ever raised for a given Thread object, then it's guaranteed that that object was started successfully. With the change, it is possible that exception is raised, and thread fails to start, leaving the object in initial state. If it were up to me, I would buy this limitation as price of safety. |
To address @Dima.Tisnek concern I have changed exception message in case thread start process is merely in progress. I kept As for testing Of course I could also do a I may be prematurely optimizing things, but people tend to like they threads to startup ASAP. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: