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

Allow the client to supply multiple ports #535

Closed
wants to merge 1 commit into from

Conversation

DanTup
Copy link
Contributor

@DanTup DanTup commented Apr 15, 2019

This extends the --port option to accept a comma-separated list of ports so that if a client has a preference for ports they can be tried in-order. We could do this in the client, but it feels a bit clumsy to keep trying to spawn processes repeatedly until it works, so I thought this would be cleaner (though if you think it's weird putting this here, we could do that).

This is working towards supporting DevTools on CrOS without Linux Chrome. It will still rely on the VM service being also being available on an exposed port (I haven't dug into that yet).

Example output below. First we bind something to port 8000:

$ dart bin/devtools.dart --port 8000
Serving DevTools at http://127.0.0.1:8000

If we attempt to bind to it again:

$ dart bin/devtools.dart --port 8000
Unable to bind to port 8000: SocketException: Failed to create server socket (OS Error: Address already in use, errno = 48), address = 127.0.0.1, port = 8000
Unable to bind to any of the supplied ports. Include 0 in the list of ports to accept a randomly assign available port.

Machine output looks like this:

$ dart bin/devtools.dart --port 8000 --machine
{"method":"server.log","params":{"level":"info","message":"Unable to bind to port 8000: SocketException: Failed to create server socket (OS Error: Address already in use, errno = 48), address = 127.0.0.1, port = 8000"}}
{"method":"server.error","params":{"message":"Unable to bind to any of the supplied ports.","fatal":true}}

If we provide additional ports, it will try them in-order:

$ dart bin/devtools.dart --port 8000,8001
Unable to bind to port 8000: SocketException: Failed to create server socket (OS Error: Address already in use, errno = 48), address = 127.0.0.1, port = 8000
Serving DevTools at http://127.0.0.1:8001

And machine output:

$ dart bin/devtools.dart --port 8000,8001 --machine
{"method":"server.log","params":{"level":"info","message":"Unable to bind to port 8000: SocketException: Failed to create server socket (OS Error: Address already in use, errno = 48), address = 127.0.0.1, port = 8000"}}
{"method":"server.started","params":{"host":"127.0.0.1","port":8001,"pid":20416}}

Finally, if you want - you can include 0 in the list to accept any port (as before):

$ dart bin/devtools.dart --port 8000,0 --machine
{"method":"server.log","params":{"level":"info","message":"Unable to bind to port 8000: SocketException: Failed to create server socket (OS Error: Address already in use, errno = 48), address = 127.0.0.1, port = 8000"}}
{"method":"server.started","params":{"host":"127.0.0.1","port":52956,"pid":20553}}

I did consider including a flag that tries the CrOS-exposed ports to avoid clients having to deal with it, but it seemed a little specific. LMK if you'd prefer that though. We could even change the default from 9100 to 8000, 8008, 808, 8085, 8888, 9005, 3000, 4200, 5000, 9100 but that also looks kinda specific. Note: as far as I can tell, there's no easy way to detect when running in a CrOS Linux container besides hostname == 'penguin' which may not be reliable.

Also something I haven't done, but we could - is make the default normally 9100,0. This would avoid failures if the port was already bound while still having a preference for 9100 to avoid the randomness if you want to bookmark/whatever.

@@ -28,8 +28,10 @@ final argParser = new ArgParser()
argPort,
defaultsTo: '9100',
abbr: 'p',
// ChromeOS exposed ports: 8000, 8008, 808, 8085, 8888, 9005, 3000, 4200, 5000
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the comment about the ports allowed on chromeos?

break; // Don't try any more if we bound one.
} on SocketException catch (e) {
printOutput(
'Unable to bind to port $port: $e',
Copy link
Contributor

Choose a reason for hiding this comment

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

only show this message if binding to all ports fail?

Copy link
Contributor

Choose a reason for hiding this comment

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

never mind. this is fine.

@DanTup
Copy link
Contributor Author

DanTup commented Apr 16, 2019

Holding off on merging this for now - as well as conflicting with @kenzieschmoll's other PR, I think there may be other blockers to running on CrOS in this way (we'd need the VM and Flutter to have some equivalent of this).

@DanTup
Copy link
Contributor Author

DanTup commented Apr 19, 2019

Haven't forgotten about this - just unclear if we'll need it now (and it doesn't make sense to land if we won't use it). If it's not clear we'll use it soon, I'll just close it.

@DanTup
Copy link
Contributor Author

DanTup commented Apr 25, 2019

There's a better solution on the way, so for now we're just going to use a single hard-coded port and don't need this.

@DanTup DanTup closed this Apr 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants