fix: handle err on both start and stop echo-server

[PedroMiguelSS]

echo-server was not handling properly error on both start() and stop() methods.

@achingbrain suggested on #565 's review:

server.start = (opts) => new Promise((resolve, reject) => {
  server.once('error', reject)
  server.listen(Object.assign({ port: defaultPort, host: '127.0.0.1' }, opts), () => {
    server.removeListener('error', reject)
    resolve()
  })
})

I think we just want to check server.listen() and server.close() related errors. For this reason, my suggestion is:

server.start = (opts) => new Promise(
  (resolve, reject) => server.listen(
    Object.assign({ port: defaultPort, host: '127.0.0.1' }, opts), (err) => err ? reject(err) : resolve()
  )
)

[achingbrain]

You need to register for the error event as well. The callback passed to .listen() is only added as a listener for the listening event.

https://nodejs.org/api/net.html#net_server_listen

[hugomrdias]

promisifing events is tricky business, do you have any example of a use-case where the error coming from .listen() isnt enough ?

[achingbrain]

My point is there will be no error passed to the callback passed to .listen().

When you start a server you'll either get a listening event (happy path) or an error event (unhappy path) - the callback passed to .listen() is only registered for the listening event.

[hugomrdias]

humm right i didn't got that the callback is just a listener to the listening event.

This makes more sense now :) plus as we are only wrapping it in a new Promise should be safe.

server.start = (opts) => new Promise((resolve, reject) => {
  server.once('error', reject)
  server.listen(Object.assign({ port: defaultPort, host: '127.0.0.1' }, opts), () => {
    server.removeListener('error', reject)
    resolve()
  })
})

[PedroMiguelSS]

@achingbrain , can you give me a second review here, please?

[hugomrdias]

the callback can be called with an Error if the server was not open

https://github.com/nodejs/node/blob/9cfb3841785cd658bb13369ed642247e089a5533/lib/net.js#L1519

[PedroMiguelSS]

I saw that on docs but I thought server.once('error', reject) would call its callback on any error (including the [ERR_SERVER_NOT_RUNNING]).

[PedroMiguelSS]

Btw, do we really need to listen error event in this case?