You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
@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)
@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.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 onwindows-latest, packaged
CI builds), this PR re-introduces thebusy_handler_timeout
but with only one resilient test that thebusy_handler_timeout
definitely does allow other working threads to progress. The negative test ofbusy_timeout
is unnecessary in addition to being flaky.