Repository navigation
Remove race conditions involving finding available ports - #18698
Conversation
|
cc @goderbauer |
|
Looks like Travis, Cirrus, and Appveyor are unhappy with this. |
|
Sigh. I was expecting the devicelab to go red, not EVERYTHING. :-) |
a56c7f1 to
ab08b02
Compare
|
Haven't started reviewing yet, but some details about the root cause and the fix in the PR description will help me to review this. |
|
The code used to have places where it looked for an available port, then passed that port to some other code and had that code open the port. That's an inherently racy operation and should never be done. This PR removes all code that did that. |
There was a problem hiding this comment.
What does it do when you give it 0?
Can't find the answer in dart --help.
There was a problem hiding this comment.
if you follow the dart code, it takes this path:
...which is documented:
https://master-docs-flutter-io.firebaseapp.com/flutter/dart-io/HttpServer/bind.html
...as "If port has the value 0 an ephemeral port will be chosen by the system." which is what we want here.
There was a problem hiding this comment.
Maybe add a comment saying the 0 will get the system to choose a free port?
There was a problem hiding this comment.
This feels pretty fragile, does the Dart team know we depend on this specific line format?
Maybe worth adding a comment where they print this information to emphasize that Flutter depends on it?
There was a problem hiding this comment.
We don't really depend on it so much as we'll just need to update the pattern when they change.
In reality, it's quite possible that there's people out there who do depend on it. Any time you change anything, you could break people.
There was a problem hiding this comment.
Maybe add a comment saying the 0 will get the system to choose a free port?
ab08b02 to
01dd6de
Compare
This is an attempt to reland flutter#18488 with less breakage on macOS.
01dd6de to
9955e25
Compare
|
Thanks for the review. Will land on green. |
4 participants
This is an attempt to reland #18488 with less breakage on macOS.