throw ToolExit when --web-port is an integer outside the valid TCP port range
#123269
Conversation
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
flutter-dashboard
bot
added
the
tool
andrewkolos
changed the title
ToolExit when --web-port is an integer outside the valid TCP port range
The error messages now originate in ResidentWebRunner, which is conceptually isolated from the CLI. While this makes the error message slightly use useful for the --web-port case, this makes it less likely for the log messages to diverge from the actual port selection logic.
Contributor
Author
|
@christopherfujino re-requesting review because of minor changes in messages and test code |
| Received a non-integer value for port: ${debuggingOptions.port} | ||
| A randomly-chosen available port will be used instead. | ||
| '''); | ||
| return globals.os.findFreePort(); |
|
|
||
| try { | ||
| return await asyncGuard(() async { | ||
| Future<int> getPort() async { |
Contributor
There was a problem hiding this comment.
Does this need to be a local function, since it's only invoked once? Can we just inline this code?
Contributor
Author
There was a problem hiding this comment.
It doesn't need to be a (local) function, but I don't think a function needs to be called more than once to justify its existence. In this case, I went with a function to
- provide a clear start and end to port selection logic,
- limit the scope of port selection logic, making it more clear that it doesn't interact with any other code within the
asyncGuardcallback, and - make it easier for first-time readers of the
runmethod by moving the low-level details of port selection out of the way (you can fold/hidegetPortand still understand whatrundoes)
amongst other things.
christopherfujino
approved these changes
Mar 24, 2023
Contributor
christopherfujino
left a comment
christopherfujino
left a comment
There was a problem hiding this comment.
Nit question about inlining getPort, but otherwise LGTM
andrewkolos
added
the
autosubmit
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 28, 2023
…valid TCP port range (flutter/flutter#123269)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 28, 2023
…valid TCP port range (flutter/flutter#123269)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 28, 2023
…valid TCP port range (flutter/flutter#123269)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 28, 2023
…valid TCP port range (flutter/flutter#123269)
This was referenced Mar 28, 2023
8 tasks
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
May 10, 2023
…valid TCP port range (flutter/flutter#123269)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
May 10, 2023
…valid TCP port range (flutter/flutter#123269)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes #123147.
Interestingly, arguing a non-integer value (
asdfor123.45) does not result in aToolExitor crash. Instead, we default to null and a randomly-selected open port is used. However, this behavior seems consistent and intentional across different*portoptions across the tool, so I left it alone.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.