Fix pipeline promise and handle errors

[robertsLando]

Fixes #20 #15

[mcollina]

Please add a test that call the callback with an error

[robertsLando]

@mcollina Tests are fine but I see a rejection error.

# without any listeners
  ok 12 reset the current messages trackers(node:4910) UnhandledPromiseRejectionWarning: Error: Connection is closed.
    at /media/daniel/DATA/Git-Projects/mqemitter-redis/node_modules/ioredis/built/connectors/StandaloneConnector.js:50:28
    at processTicksAndRejections (internal/process/task_queues.js:76:11)
(node:4910) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:4910) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the ✓ without any listenersnon-zero exit code.

That rejection is thrown when emit throws error

[mcollina]

Code looks good, but the unhandledRejection should be fixed.

[robertsLando]

@mcollina The problem with the test that never exit is in aedes-persistence abstract test, some tests doens't call inbstance.destroy at the end. I will submit a PR for this

handledRejection should be fixed.

I have no clue who is sending that rejection, It should not be thrown because I catch the error correctly but seems that when I call that.state.emit('error', err) this throws the unhandled rejection with the error emitted. Is there a reason for this? If instead of 'error' I use another event name I don't get that

UPDATE: I have found this: #17

[robertsLando]

@mcollina Now it is fixed! This fix also #15

[robertsLando]

@mcollina Please make a new release after this merge so I can also fix this in aedes-persistence-redis

[robertsLando]

@mcollina Could you review this and if ok merge and release a new version? So I can go on with aedes-persistence-redis PR

[mcollina]

lgtm

[mcollina]

why its this not error?

[robertsLando]

@mcollina Because 'error' throws unhandled rejections on emit, check #15 and #21 (comment)

[mcollina]

I think we need to track down why this is happening wqithout changing event name.

[robertsLando]

@mcollina I have no clue Matteo... I have try to debug this but I can only tell you when this happens:

If emit throws an error and cb is not defined the unhandled rejection is thrown after I call this.state.emit('error'), it is like that emit throws an error instead of just emit it

[robertsLando]

Ok I have found this in docs: https://nodejs.org/api/events.html#events_class_events_eventemitter

If an EventEmitter does not have at least one listener registered for the 'error' event, and an 'error' event is emitted, the error is thrown, a stack trace is printed, and the Node.js process exits.

So the solutions are: change the event name or create a default event listener for 'error' (noop) like I did in my latest commit 464e7ed

[mcollina]

The problem is not avoiding the unhandledRejection, but rather understading where it come from and handle it there.

[robertsLando]

@mcollina It comes from: https://github.com/mcollina/mqemitter/blob/master/abstractTest.js#L139

Close is called immediatly after emit and this causes that error. Should I fix that test? If yes, how? I mean I could add an on listener and close the connection after the message is received but if so the previous test already does this. Anyway if a user doesn't add an 'error' listener this will throw the error anyway, is this the expected behaviour?

[mcollina]

Which operation is missing a catch() handler? That's what is not clear to me.

[robertsLando]

If emit throws an error and cb is not defined the unhandled rejection is thrown after I call this.state.emit('error'), it is like that emit throws an error instead of just emit it

Please check my prev comment https://github.com/mcollina/mqemitter-
redis/pull/21#discussion_r385634852

In the abstract test it is thrown here: https://github.com/mcollina/mqemitter/blob/master/abstractTest.js#L148

Because this test throws close after emit and this will make pipeline in mqemitter redis to throw an error that is catched but once I call 'emit('error')` on the catch this thows an error as there is no error listener and this is the default behaviour of event emitter

[mcollina]

Which promise is rejecting? Which line of code creates that promise? Why does it not have a .catch() handler attached?

[robertsLando]

@mcollina This is the promise:

https://github.com/robertsLando/mqemitter-redis/blob/464e7ed5d6bbf329e352946748e92cca63fa7306/mqemitter-redis.js#L164

It has a catch block that calls done that in this case is not a user callback but will be _onError that will call emit('error') that will throw the unhandled rejection, the unhandled rejection happens on the catch block this is why it is not catched

https://github.com/robertsLando/mqemitter-redis/blob/464e7ed5d6bbf329e352946748e92cca63fa7306/mqemitter-redis.js#L37-L41

[mcollina]

LGTM

[robertsLando]

@mcollina Remember to create a release after the merge so I can go on with aedes-persistence-redis 😄