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

Fix race condition in servodriver #16819

Merged
merged 1 commit into from May 15, 2019
Merged

Conversation

@georgeroman
Copy link
Contributor

georgeroman commented May 14, 2019

Fixes #16754

self.webdriver_port = get_free_port(4444, exclude=self.used_ports)
self.used_ports.add(self.webdriver_port)
finally:
ServoWebDriverBrowser.used_ports_lock.release()

This comment has been minimized.

Copy link
@jdm

jdm May 14, 2019

Contributor

Let's do this instead:

with ServoWebDriverBrowser.used_ports_lock:
    self.webdriver_port = get_free_port(4444, exclude=self.used_ports)
    self.used_ports.add(self.webdriver_port)

This comment has been minimized.

Copy link
@jugglinmike

jugglinmike May 14, 2019

Contributor

You beat me to it!

@jdm
jdm approved these changes May 14, 2019
@gsnedders
Copy link
Contributor

gsnedders commented May 14, 2019

I wonder if every usage of get_free_port is racy in the face of parallel test execution?

@georgeroman
Copy link
Contributor Author

georgeroman commented May 15, 2019

I think it is since every usage of the function follows the same pattern of getting the first available port followed by saving the chosen port in excluded_ports.

@jdm jdm merged commit d04f026 into web-platform-tests:master May 15, 2019
12 checks passed
12 checks passed
Azure Pipelines Build #20190514.108 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests (Safari Technology Preview)) affected tests (Safari Technology Preview) succeeded
Details
Azure Pipelines (affected tests without changes (Safari Technology Preview)) affected tests without changes (Safari Technology Preview) succeeded
Details
Azure Pipelines (infrastructure/ tests (macOS)) infrastructure/ tests (macOS) succeeded
Details
Azure Pipelines (tools/ unittests (macOS)) tools/ unittests (macOS) succeeded
Details
Azure Pipelines (tools/wpt/ tests (macOS)) tools/wpt/ tests (macOS) succeeded
Details
Azure Pipelines (tools/wptrunner/ unittests (macOS)) tools/wptrunner/ unittests (macOS) succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Taskcluster (pull_request) TaskGroup: success
Details
wpt.fyi - safari[experimental] Safari results
Details
@SimonSapin
Copy link
Contributor

SimonSapin commented May 16, 2019

Binding to port 0 lets the OS pick an available port at semi-random, without the race. getsockname can then be used to find out which port was used.

@jgraham
Copy link
Contributor

jgraham commented May 16, 2019

I seem to recall that there's a problem doing that, at least with the way that marionette works, because we need to pass the port number in to the command line argument rather than bind it ourselves. So we could get a socket with a random/unused port then read it's port number and then pass that down after closing it but we can't get a socket on a random port and then use that socket directly.

bors-servo added a commit to servo/servo that referenced this pull request May 23, 2019
…th_tests, r=jdm

Reintroduce parallelism in the bluetooth tests

<!-- Please describe your changes on the following line: -->
With web-platform-tests/wpt#16819 in place, #22619 should be fixed, so the changes introduced by #22662 can be reverted.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23407)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.