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

Handle failure during thread creation #2471

Merged

Conversation

antonpirker
Copy link
Member

@antonpirker antonpirker commented Oct 30, 2023

In Python 3.12 when you try to start a thread during shutdown a RunTimeErrror is raised. Handle this case with grace

@antonpirker antonpirker marked this pull request as ready for review October 31, 2023 08:32
Copy link
Member

@szokeasaurusrex szokeasaurusrex left a 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

Comment on lines 926 to 927
self.running = False
return
Copy link
Member

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.

Copy link
Contributor

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)

sentry_sdk/sessions.py Show resolved Hide resolved
sentry_sdk/monitor.py Show resolved Hide resolved
@antonpirker antonpirker mentioned this pull request Oct 31, 2023
@sentrivana sentrivana self-assigned this Nov 2, 2023
@sentrivana sentrivana added this to the Python 3.12 Support milestone Nov 2, 2023
@sentrivana sentrivana mentioned this pull request Nov 2, 2023
4 tasks
Copy link
Member

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve

@sentrivana sentrivana enabled auto-merge (squash) November 2, 2023 14:21
@sentrivana sentrivana merged commit 5ddc1e7 into master Nov 2, 2023
284 of 285 checks passed
@sentrivana sentrivana deleted the antonpirker/handle-creation-of-threads-during-shutdown branch November 2, 2023 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants