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

Handle lack of port more gracefully? #145

Open
jpw opened this issue Nov 10, 2016 · 6 comments
Open

Handle lack of port more gracefully? #145

jpw opened this issue Nov 10, 2016 · 6 comments
Assignees

Comments

@jpw
Copy link
Contributor

jpw commented Nov 10, 2016

If port is not specified we get a spawn loop. Not great I guess, but not sure if this is a thing to be fixed.

Maybe we should catch this and halt?

$ node app -p --route-override=$BACKEND_APP
...
internal/net.js:17
    throw new RangeError('"port" argument must be >= 0 and < 65536');
    ^

RangeError: "port" argument must be >= 0 and < 65536
    at assertPort (internal/net.js:17:11)
    at Server.listen (net.js:1383:5)
    ...
    at Module._compile (module.js:541:32)
    at Object.Module._extensions..js (module.js:550:10)
    at Module.load (module.js:458:32)
    at tryModuleLoad (module.js:417:12)
2016-11-10T12:23:15.638Z - error: 31131 died with error code 1, starting new worker...
...
@andrewmee
Copy link
Contributor

I thought we defaulted to port 80 (for http). Let's have a look at this and see how we're getting into this loop.

@hollsk
Copy link
Contributor

hollsk commented Nov 10, 2016

See #103 - we should be accepting port 80 as the default.

@andrewmee
Copy link
Contributor

Oh I've just realised, you're explicitly calling -p for port, but not providing one.

That would probably explain why it's not using the default. I guess we could do a better job of just throwing the error once instead of looping, but an error it would be.

@jpw
Copy link
Contributor Author

jpw commented Nov 10, 2016

Thinking some more, should we apply 80 if port number is unspecified and warn the user, or exit?

@hollsk
Copy link
Contributor

hollsk commented Nov 10, 2016

Just exiting would be unexpected behaviour, IMO. Port numbers are so well-known that I think most people expect applications to have some insight as to their intentions, even when they're not explicitly stated.

Notifying the user that 80 is being defaulted to would be reasonable, though.

@jpw jpw self-assigned this Mar 24, 2017
@josebolos
Copy link
Contributor

josebolos commented Oct 19, 2017

I can't seem to be able to replicate this issue using just Shunter. I'm assuming this is an app using shunter that is failing in this manner?

That said, the behaviour with shunter itself is quite inconsistent at the moment. For example:

  1. No port specified:
$ node ./bin/serve.js
Shunter Serve started on http://localhost:5401
^C
  1. -p specified but empty:
$ node ./bin/serve.js -p
Shunter Serve started on http://localhost:3000
^C
  1. -p 80 specified (fails as expected due to not being root):
$ node ./bin/serve.js -p 80
events.js:182
      throw er; // Unhandled 'error' event
      ^

Error: listen EACCES 0.0.0.0:80

Behaviour 2 is wrong and needs to be corrected. An invalid or incomplete argument string being passed to an app is an error, this is standard behaviour on every UNIX app that I know:

Valid argument string:

$ ls --color=never
HISTORY.md  Makefile   bin       docs  filters  logging       package.json  tests
LICENSE     README.md  coverage  dust  lib      node_modules  public        view

Invalid argument string:

$ ls --color=muffin
ls: invalid argument ‘muffin’ for ‘--color’
Valid arguments are:
  - ‘always’, ‘yes’, ‘force’
  - ‘never’, ‘no’, ‘none’
  - ‘auto’, ‘tty’, ‘if-tty’
Try 'ls --help' for more information.

I'm going to have a go at fixing scenario 2 by making it error out. We can see then how it affects Jon's scenario.

@josebolos josebolos assigned josebolos and unassigned jpw Oct 19, 2017
josebolos added a commit that referenced this issue Oct 19, 2017
When an optional parameter like `-p` that requires an argument is used but an argument is not passed, this will cause the app to fail as expected instead of using a random value. For example, port was being set to 3000 instead of the default 5401.

This hopefully addresses #145.
josebolos added a commit that referenced this issue Jan 21, 2020
When an optional parameter like `-p` that requires an argument is used but an argument is not passed, this will cause the app to fail as expected instead of using a random value. For example, port was being set to 3000 instead of the default 5401.

This hopefully addresses #145.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants