Fix 104 improve error handling

[stansv]

Fixed some cases when UnhandlerPromiseRejectionWarning's can appear, the simplest solution is to do console.log to handle rejections. Emitting error events in catch() sections would also produce UnhandlerPromiseRejectionWarning (it was surprising for me). Probably it'd be better to modify instance state (write error details into some field) and make all its methods non-functional by re-throwing that error in each call, but this is not very suitable for worker which can be used without calling any instance methods.

Also I've moved array of worker tokens to worker class field level, this can be helpful for monitoring.

Fixes #104

[dirkgroenen]

Shouldn't the errors be emitted instead of logged, allowing end-users to catch them and act accordingly?

Something like this.run().catch(this.emit.bind(this)); instead of logging them on the console.

[manast]

I think it would be better as suggested in the comments to emit the errors instead of just logging them out

[stansv]

Hi guys, yes I think suggested change is a good idea, but I'm not completely understand how to create an event listener for these errors.
PS I've also tried to do this.emit.bind(this)('error', error) before, but this event (error) causes UnhandlerPromiseRejectionWarning if no listeners attached..

[manast]

@stansv are you sure about that? emitting an error thrown by a promise should avoid the warning you are mentioning.

[stansv]

@manast

import { EventEmitter } from 'events';

class Test extends EventEmitter {
  constructor() {
    super();
    Promise.reject(new Error('test')).catch(error => {
      this.emit('error', error);
    })
  }
}

(() => {
  new Test();
})();

$ ts-node src/index.ts
(node:15135) UnhandledPromiseRejectionWarning: Error: test
    at new Test (/Users/stansv/bullmq/src/index.ts:11:20)
    at /Users/stansv/bullmq/src/index.ts:18:3

(as I noted previously event name must be error to get this UnhandledPromiseRejectionWarning)

I feel I missing something about promises..

[ccollie]

Could be node's scheduling of the catch. In many promise implementations it's delayed at least until the next tick of the event loop. In your case the end of the program may have been reached before the event fired.

[stansv]

No, I've found the reason — emit('error') itself throws an Error if there's no registered 'error' event listener (this is documented). Throwing in catch() block in turn causes UnhandledPromiseRejectionWarning.
The "correct" code would be

import { EventEmitter } from 'events';

class Test extends EventEmitter {
  constructor() {
    super();
    Promise.reject(new Error('test')).catch(error => {
      try {
        this.emit('error', error);
      } catch(_) {
        // ignore...
      }
    })
  }
}

(() => {
  new Test();
})();

[stansv]

@manast thanks for approval, but I cannot agree with current behavior, namely with

this.run().catch(this.emit.bind(this));

— if emit is called then first arg must be event name, not arbitrary error object; I do not understand how this might work (@dirkgroenen maybe you can help). How about this?

this.run().catch(error => {
      try {
        this.emit('error', error); // emit exactly 'error' event
      } catch(error) { // if 'error' event listener was not specially attached,
        console.log(error); // just print error to stdout
      }
    })

[dirkgroenen]

— if emit is called then first arg must be event name, not arbitrary error object; I do not understand how this might work (@dirkgroenen maybe you can help). How about this?

You're right @stansv , I think I made a typo and was supposed to write this.run().catch(this.emit.bind(this, 'error'));

/ My two cents; you might want to consider logging the error with an additional message informing about the missing error handler?

[manast]

Going to close this PR as master has drifted too much away from this branch and it is not easy to merge anymore.