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

Get the elements from queue in block mode #34333

Merged
merged 5 commits into from
Jun 24, 2022
Merged

Get the elements from queue in block mode #34333

merged 5 commits into from
Jun 24, 2022

Conversation

WeizhongX
Copy link
Contributor

Previously there are cases a worker get no more tests to run
and quit early where there are still groups in the queue. This
is because poll from Queue in unblock mode is not safe.

Now poll in block mode and add a sentinal for each worker at
the end of queue.

Previously there are cases a worker get no more tests to run
and quit early where there are still groups in the queue. This
is because poll from Queue in unblock mode is not safe.

Now poll in block mode and add a sentinal for each worker at
the end of queue.
@wpt-pr-bot wpt-pr-bot added infra wptrunner The automated test runner, commonly called through ./wpt run labels Jun 7, 2022
tools/wptrunner/wptrunner/testloader.py Outdated Show resolved Hide resolved
@@ -383,7 +383,7 @@ def group_metadata(cls, state):
def group(self):
if not self.current_group or len(self.current_group) == 0:
try:
self.current_group, self.current_metadata = self.test_queue.get(block=False)
self.current_group, self.current_metadata = self.test_queue.get(block=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm quite worried by this blocking forever; it seems to create a lot of potential for a bug to cause a hang. I'm not sure why reading a populated queue without blocking should be failing, but at the least there should be some kind of timeout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi James, I updated per your suggestion. Can you pls review again? Thanks!

@WeizhongX WeizhongX closed this Jun 14, 2022
@WeizhongX WeizhongX reopened this Jun 14, 2022
Copy link
Contributor

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

OK, with the minor grammar fix.

I'm going to propose changing when we create the test source so we can pass the logger in, but we can do that once this has landed.

tools/wptrunner/wptrunner/testloader.py Outdated Show resolved Hide resolved
@WeizhongX WeizhongX closed this Jun 22, 2022
@WeizhongX WeizhongX reopened this Jun 22, 2022
@WeizhongX WeizhongX closed this Jun 23, 2022
@WeizhongX WeizhongX reopened this Jun 23, 2022
@WeizhongX WeizhongX closed this Jun 24, 2022
@WeizhongX WeizhongX reopened this Jun 24, 2022
@WeizhongX WeizhongX merged commit e857c01 into master Jun 24, 2022
@WeizhongX WeizhongX deleted the fix-poll branch June 24, 2022 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra wptrunner The automated test runner, commonly called through ./wpt run
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants