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

Better errors descriptions for listen(); #15

Merged
merged 7 commits into from Jan 27, 2017

Conversation

Projects
None yet
2 participants
@morozRed
Copy link
Contributor

commented Jan 23, 2017

Summary:

  • Add error-helper.js
  • Add error-helper.test.js
@ai

This comment has been minimized.

Copy link
Member

commented Jan 23, 2017

Good start. But if we want good docs for new users we should have better error message.

Let’s think together. Why user will get Port already in use error? 31337 and 1337 ports are very rare. So it is very rare case that some other non-Logux program use this port.

In my experience there are 2 most popular cases:

  1. User have 2 Logux projects. He worked on first. And now manager asked him to fix some issue in second project. And developer forget to stop first Logux server before run second Logux server.
  2. You deploy new Logux app to servers. But previous app was not killed correctly because of error in deploy script.

In all that cases “Try to use other port” will make only worse.

So, let’s change error text. You could try start with port in use case. Describe (with simple language) what is the problem. Describe possible reason why it happens. For each reason describe how to solve it.

You can start here. Just write it in comment.

@morozRed

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2017

Thanks!
Ok, we can do something like this:
EADDRINUSE

ERROR   Port :3000 already in use.
HINT    Looks like Logux server already running on this port.
        Make sure there is no Logux servers running on this port.

EACCES

ERROR  You are not allowed to run server on this port.
HINT   Try to run server on port > 1024.
       You are currently using port: ${port}.
@ai

This comment has been minimized.

Copy link
Member

commented Jan 24, 2017

Yeap, much better. But let’s write more details:

ERROR   Port :3000 already in use.
        Another Logux server or other app already running on this port.
        Maybe you didn’t not stop server from other project
        or previous version of this server was not killed.

Now you need to do same work with EACCES. Explain user why <= 1024 posrt are different. What is the main reason of EACCES error?

@morozRed

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2017

What about this?
EACCES

ERROR  You are not allowed to run server on this port.
HINT   You are not allowed to run your server on ports <= 1024
       Because they are privileged by security reasons.
       Try to run your server on ports > 1024.
@ai

This comment has been minimized.

Copy link
Member

commented Jan 24, 2017

@morozRed do write docs without analysis ;). Tell me possible scenarios when users will have EACCES error.

@morozRed

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2017

  1. User did something like like this in his app
 app.listen({
    port: 1000
}) 
  1. User is trying to access a file in a way forbidden by its file access permissions
@ai

This comment has been minimized.

Copy link
Member

commented Jan 24, 2017

  1. Why he or she will do it? I think most of users will copy code from docs ;).
  2. Other reason — developer forget to write sudo to start server. I had this mistake for my own too :D.
  3. EACCES could be for files too?!!! In this case we should detect port and file EACCES.
@morozRed

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2017

  1. Hope they will just copy it :)
  2. my bad
  3. Yes, I just found it here: https://nodejs.org/api/errors.html#errors_error_code

I'll find a way we can detect EACCES type :) And then we will discuss error HINT again

@morozRed

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2017

After little investigation I've realised that EACCES for files will not appear on listen() call. So we can finalize error message:

ERROR  You are not allowed to run server on this port
       Try to run server with sudo prefix or use port >= 1024
@ai

This comment has been minimized.

Copy link
Member

commented Jan 24, 2017

Listen could load certificate files for TLS. Maybe we need good message for them.

@morozRed

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2017

I've tested it and got this error:

ERROR  EACCES: permission denied, open '/server.key' at 2017-01-24 17:23:42

{ 
  Error: EACCES: permission denied, open '/server.key'
  errno: -13,
  code: 'EACCES',
  syscall: 'open',
  path: '/server.key' 
}

I'm not sure if this is a good idea to handle EACCES for files here, because if you will try to open privileged file while server working the same error will appear.

@ai

This comment has been minimized.

Copy link
Member

commented Jan 24, 2017

We need to cover this case too with good instructions.

@ai

This comment has been minimized.

Copy link
Member

commented Jan 24, 2017

By the way, if try to add sudo is good recommendation. Running server as root is very very dangerous. Maybe we should ask “try to change user”?

@morozRed

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2017

Ok, I'll think about this case with certificate.
Yes, probably it's better.

@ai

This comment has been minimized.

Copy link
Member

commented Jan 24, 2017

OK, now let’s update text in PR and I will accept it.

@ai

This comment has been minimized.

Copy link
Member

commented Jan 24, 2017

Few things to fix in code:

  1. Use some real port instead of undefined in tests.
  2. Move common code from error-helper and reporter to some common file.
  3. Add real integration test. You can use some examples from test/servers.js tests. Just run 2 servers in same time to get port in use error.
  4. Instead of e.syscall === 'listen' hack just use try-catch. Simple solutions is always better. what if some user’s methods will be named listen too?

morozRed added some commits Jan 25, 2017

@morozRed

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2017

Updates:

  • updated errors messages
  • add tests
  • fix old tests
  • add log-helper.js
process.stderr.write(errorHelper(e, app))
} catch (err) {
app.reporter('runtimeError', app, undefined, err)
}

This comment has been minimized.

Copy link
@ai

ai Jan 25, 2017

Member

You misunderstand me. onError should contains only app.reporter('runtimeError', app, undefined, e).

I told you to write try-catch in listen() method:

listen: function listen () {
  try {
    return BaseServer.prototype.listen.apply(this, arguments)
  } catch (e) {
    process.stderr.write(errorHelper(e, app))
  }
}

@ai ai merged commit 4204304 into logux:master Jan 27, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.