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

Execute job in a domain and catch uncatched exception to mark job as failed #403

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lchenay
Copy link

@lchenay lchenay commented Sep 5, 2014

Execute job in a domain and catch uncatched exception to mark job as failed

Uncatched exception make kue to stop, and make job stuck in unknown statement.

Related to #391

…failed.

Uncatched exception make kue to stop, and make job stuck in unknown statement.

Related to Automattic#391
@behrad
Copy link
Collaborator

behrad commented Sep 5, 2014

Kue client codes can simply wrap their process in a domain or execute that with a promise based library.
Using domain in Kue internally is something we should care about since it caughtes all exceptions hiding them from client's code execution context.
I think marking a job failed on uncaught exception on user's code is to much assumption!

@lchenay
Copy link
Author

lchenay commented Sep 5, 2014

Client code have to be aware of the problem and the solution to wrap their process in a domain. And trust us, we are not.

Moreover, if these exception are not catched, node will be terminate and all jobs will be stop without any warning. All day many job stuck due to this.

I add a test, it do not caughtes all exception hiding them from client, it only catch uncatched exception.

I do agree on client should protect their code with try catch, promise or err in callback, but we use many libraries, and not all of them are protected of uncatched error.

I will just quote an article read on this topic:

An application without uncaught exceptions

This opinion is completely bizarre to me. Applications will always have exceptions at some stage and will probably have uncaught exceptions. If you hold this opinion you push error handling onto your users and are likely to get some late night calls that the server has gone down.
http://shapeshed.com/uncaught-exceptions-in-node/

Here a gist for the same patch as external patch, that have to be require() before using kue if you do not want jobs to be stuck due to bad library. https://gist.github.com/lchenay/2ca7651a0f57f50ce672

@behrad
Copy link
Collaborator

behrad commented Sep 5, 2014

Moreover, if these exception are not catched, node will be terminate and all jobs will be stop without any warning. All day many job stuck due to this.

This is my fault not to mention this in documentation and suggest using domains/... to properly make your node.js process robust.

it only catch uncatched exception

and the problem is the same, it hides uncaught exceptions from client's code, making their app very difficult to debug. This is the high most (application) layer code which should handle most of the exceptions, So I won't (i had the same patch ready for about 6 months in a private branch) add this until I find/am suggested proper solution to pass the exception to client code, or at least give them fully exception stack/context so that they can be informed of what is going wrong.

@behrad
Copy link
Collaborator

behrad commented Sep 5, 2014

kriskowal/q#120

@lchenay
Copy link
Author

lchenay commented Sep 6, 2014

Client are aware of failed job in this case:

  • Job will be marked as failed in UI, and they will have the exception attached to the job
  • Clients can track failed job with "failed" event like in normal error handling with done("failed job");
  • Error is logged in kue log

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

Successfully merging this pull request may close these issues.

2 participants