-
Notifications
You must be signed in to change notification settings - Fork 517
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
Handle failure during thread creation #2471
Handle failure during thread creation #2471
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
sentry_sdk/profiler.py
Outdated
self.running = False | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve
In Python 3.12 when you try to start a thread during shutdown a
RunTimeErrror
is raised. Handle this case with grace