gh-106236: Replace assert with raise RuntimeError in threading.py
#106237
Conversation
There was a problem hiding this comment.
Assert statements should only be used to catch internal errors, for debugging, not user errors. So I consider the use of assert and raising of AssertionError in _DummyThread to be an error from the beginning. To fully fix this, another error should be raised instead. (Yes, get other opinions.) Which can be decided once agreed on not AssertionError.
.is_alive may be a different case. It checks the value of private variables, and unless the user can change those private variables through public interfaces, (which theorhetically should not be possible), rather than directly, the assert there would seem proper as is. IE, an AssertionError would indicate a CPython bug rather than a user bug.
The base problem here is that _DummyThread is a Thread- rather than a Thread+.
Misc/NEWS.d/next/Library/2023-06-29-15-10-44.gh-issue-106236.EAIX4l.rst
Outdated
Show resolved
Hide resolved
Lib/threading.py
Outdated
|
|
||
| def join(self, timeout=None): | ||
| assert False, "cannot join a dummy thread" | ||
| raise AssertionError("cannot join a dummy thread") |
There was a problem hiding this comment.
I think another error should be raised. See review comment.
Lib/threading.py
Outdated
| return True | ||
| if not self._is_stopped and self._started.is_set(): | ||
| return True | ||
| raise AssertionError("thread is not alive") |
There was a problem hiding this comment.
I am not sure that any change is needed here. If so, I think another error. See review comment.
There was a problem hiding this comment.
I think it is needed. _DummyThread is assumed to always be alive, but as I showed in the test case, it is easy to kill it (sort of). So, adding an error about reaching an unwanted state seems like a good thing to do.
I agree that AssertionError is probably not the best way to raise an error, will change it to RuntimeError.
There was a problem hiding this comment.
Again, the counter example would be:
import threading, _thread
def f(mutex):
threading.current_thread()
mutex.release()
mutex = threading.Lock()
mutex.acquire()
tid = _thread.start_new_thread(f, (mutex,))
mutex.acquire()
threading._active[tid]._started.clear()
threading._active[tid].is_alive()With asserts on main it will raise AssertionError
With -OO on main it will return True
I think that this is not fine.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase And if you don't make the requested changes, you will be put in the comfy chair! |
…AIX4l.rst Co-authored-by: Terry Jan Reedy <[email protected]>
|
@terryjreedy thanks a lot for the quick review! Addressed all your comments :) I have made the requested changes; please review again |
|
Thanks for making the requested changes! @terryjreedy: please review the changes made to this pull request. |
Misc/NEWS.d/next/Library/2023-06-29-15-10-44.gh-issue-106236.EAIX4l.rst
Outdated
Show resolved
Hide resolved
…AIX4l.rst Co-authored-by: Alex Waygood <[email protected]>
AlexWaygood
changed the title
assert with raise AssertionError in threading.pyassert with raise RuntimeError in threading.py
|
@gpshead The issue was marked as a bug because using assert caused code to misbehave. If you agree, this should be backported. |
|
If we were still in the beta period i'd have just hit the backport button for 3.12. But the next release is 3.12rc1 and I'd rather reserve changes I merge there to important things. This one doesn't seem like an important enough bug fix to bother on (it has existed forever and I'm not aware of anyone actually noticing it in an actual application). If someone sees a need to backport it, no objection from me, I'm not going to bother myself. |
I went with
AssertionErrorso we maintain 100% backward compatibility._DummyThreads can be joined in-OOmode #106236