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

Don't attempt to register signals in non-main thread #11548

Merged
merged 2 commits into from
Jan 4, 2024

Conversation

serinamarie
Copy link
Contributor

@serinamarie serinamarie commented Jan 4, 2024

This helps address #10895
The flaky tests on main leading to: ValueError: signal only works in main thread of the main interpreter are getting out of hand. This PR attempts to fix that by only attempting to registering signal on main thread.

Checklist

  • This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • This pull request includes tests or only affects documentation.
  • This pull request includes a label categorizing the change e.g. fix, feature, enhancement, docs.

For documentation changes:

  • This pull request includes redirect settings in netlify.toml for files that are removed or renamed

Copy link

netlify bot commented Jan 4, 2024

Deploy Preview for prefect-docs-preview ready!

Name Link
🔨 Latest commit d8bc174
🔍 Latest deploy log https://app.netlify.com/sites/prefect-docs-preview/deploys/6596cf301850b40008e43ef5
😎 Deploy Preview https://deploy-preview-11548--prefect-docs-preview.netlify.app/api-ref/server/services/loop_service
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@serinamarie serinamarie added the development Tech debt, refactors, CI, tests, and other related work. label Jan 4, 2024
@serinamarie serinamarie changed the title Attempt to fix flaky test Don't register signals in thread Jan 4, 2024
@serinamarie serinamarie marked this pull request as ready for review January 4, 2024 01:09
@serinamarie serinamarie requested a review from a team as a code owner January 4, 2024 01:09
@serinamarie serinamarie changed the title Don't register signals in thread Don't attempt to register signals in non-main thread Jan 4, 2024
@zzstoatzz
Copy link
Collaborator

@serinamarie hmm - what do you think will happen when the tests that previously failed with signal only works in main thread encounter this and do not end up calling signal.signal / just no op? or do you think we are erroneously getting to that signal.signal call in the first place?

@serinamarie
Copy link
Contributor Author

@serinamarie hmm - what do you think will happen when the tests that previously failed with signal only works in main thread encounter this and do not end up calling signal.signal / just no op?

Tests that previously intermittently failed on this error would not fail on the error and the tests would still run. It's just that the signal wouldn't be registered in those cases. It doesn't seem to have affected the ability of the tests to run/pass.

or do you think we are erroneously getting to that signal.signal call in the first place?

I don't think it's necessarily erroneous, just that the tests themselves are sometimes run off the main thread and it's an underlying issue with pytest.

@serinamarie serinamarie requested a review from abrookins January 4, 2024 01:45
@serinamarie
Copy link
Contributor Author

@abrookins I think there's more to do here but the errors are becoming more prevalent so this is my attempt to get something across the line. But wondering if you have thoughts.

Copy link
Contributor

@abrookins abrookins left a comment

Choose a reason for hiding this comment

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

Let's do it. Whatever the reason we might get here and try to register a signal handler off the main thread, pytest-related or otherwise, it's not going to work, so we may as well prevent it.

Copy link
Collaborator

@chrisguidry chrisguidry left a comment

Choose a reason for hiding this comment

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

Since we've never observed this error anywhere but tests, or had any users report encountering it, it makes sense to me that we'd just quietly skip this.

@serinamarie serinamarie requested a review from zangell44 as a code owner January 4, 2024 15:30
@serinamarie serinamarie merged commit 70dd469 into main Jan 4, 2024
56 checks passed
@serinamarie serinamarie deleted the try-to-fix-signal-handling branch January 4, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Tech debt, refactors, CI, tests, and other related work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants