Handle failure during thread creation#2471
Conversation
szokeasaurusrex
left a comment
There was a problem hiding this comment.
Please see my comment on profiler.py; it applies to all the files.
I also noticed an ensure_running function in the GeventScheduler of profiler.py that was unchanged in this PR, even though we are spawning a thread there. Does this method also need to be updated, or is a RuntimeError impossible here?
If possible, we might also add some tests to ensure this behavior works correctly
| self.running = False | ||
| return |
There was a problem hiding this comment.
I find it slightly confusing here that the method is called ensure_running, but it is possible for the method to terminate without the thread running. We should have some way to indicate failure, perhaps by raising a custom exception when ensure_running cannot start the scheduler or by returning a value to indicate success/failure. We might instead or additionally consider renaming the function to indicate that we may fail to ensure that the scheduler is running and/or add a documentation comment.
If we make this change to indicate when the ensure_running fails, we should also make sure to handle failure appropriately in the calling code.
There was a problem hiding this comment.
@szokeasaurusrex Added docstrings to the three ensure_running functions.
I considered making the function return a bool, but in all three cases it already communicates success/failure by setting the self.(_)running variable, so it feels a bit redundant to make it return essentially the same information, especially since the calling places usually can't do much with the return value -- all code that depends on ensure_running being run checks the value of self.(_)running instead anyway. Which feels like an ok pattern to me.
Regarding the other points:
- added some tests
- added the same logic to the gevent scheduler (I checked and it would also attempt to spawn a thread)
In Python 3.12 when you try to start a thread during shutdown a
RunTimeErrroris raised. Handle this case with grace