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

busy_handler_timeout pt2 #456

Merged

Conversation

fractaledmind
Copy link
Contributor

@fractaledmind fractaledmind commented Jan 4, 2024

Since #443 required a revert (#457) because the test for busy_timeout holding the GVL such that other working threads could not progress was flaky (particularly on windows-latest, packaged CI builds), this PR re-introduces the busy_handler_timeout but with only one resilient test that the busy_handler_timeout definitely does allow other working threads to progress. The negative test of busy_timeout is unnecessary in addition to being flaky.

@fractaledmind
Copy link
Contributor Author

Debugging now.

@fractaledmind
Copy link
Contributor Author

fractaledmind commented Jan 4, 2024

@flavorjones: It seems that windows-latest, packaged does release the GVL with busy_timeout 😳:

["busy_handler_timeout", [".", ".", ".", ".", ".", ".", ".", ".", ".", ">", ".", ".", ".", ".", ".", ".", ".", ".", "."]]
["busy_timeout", [".", ".", ".", ".", ".", ".", ".", ".", ".", ".", ".", ".", ".", ".", ".", ".", ".", ".", ">", ".", ".", ".", ".", ".", ".", ".", ".", ".", ".", ".", ".", ".", ".", ".", "."]]

Do you see something wrong in the tests, or is Windows just behaving differently with threads?

@flavorjones
Copy link
Member

maybe @tenderlove has time to take a look?

@fractaledmind
Copy link
Contributor Author

You know what, why do we need a test that the busy_timeout holds the GVL? That isn't actually necessary. The test that busy_handler_timeout allows the work thread to work while retrying is the test that is necessary. I'm just going to delete the other test.

@fractaledmind
Copy link
Contributor Author

@flavorjones: Removing that test, as fully expected, brings us to green. I see you just reverted, so I will need to add back the actual method, but that is easy enough. If you agree that the one test about busy_timeout is actually superfluous, I will add the method back, keep the new test, and we can move forward with that. Let me know and I will do this tomorrow (headed to bed now)

@fractaledmind
Copy link
Contributor Author

@flavorjones @tenderlove This adds the code back with a single robust test that asserts the new code behaves how we want. Should be good to go

@fractaledmind fractaledmind changed the title Fix busy_handler_timeout tests busy_handler_timeout pt2 Jan 7, 2024
@fractaledmind
Copy link
Contributor Author

@flavorjones @tenderlove: Is there anything I need to do to get this reviewed? Not trying to be pushy, only proactive. If you simply are busy in the short-term and can't review PRs in this repo, I understand. I'm only pinging because I know as a maintainer on a level probably with 10x or 100x fewer notifications, it is easy to "mark something as read" and then it slips into the ever-flowing stream of notifications and things to consider.

@tenderlove
Copy link
Member

@fractaledmind no worries, thanks for pinging. CI is green and the patch seems good, so I'll merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants