FirebaseExtended / firebase-queue

MIT License
786 stars 108 forks source link

Asynchronous errors cause workers to hang in the "busy" state #39

Closed dylanjha closed 8 years ago

dylanjha commented 8 years ago

If an asynchronous error is thrown, the worker stays in the "busy" state. If workers stay in the "busy" state they are not free to pick up more jobs off the queue

Example 1 - synchronous error (works smoothly)

If a synchronous error is thrown the job automatically (and correctly) gets rejected reject() (commit thanks @drtriumph !)

var queue = new Queue(ref, function(data, progress, resolve, reject) {
  // Read and process task data
  console.log(data);

  // synchronous error
  throw new Error("Something went wrong")
});

Example 2 - asynchronous error (worker hangs in the "busy" state)

If an asynchronous error is thrown the job hangs and never gets cleaned up.

var queue = new Queue(ref, function(data, progress, resolve, reject) {
  // Read and process task data
  console.log(data);

  // asynchronous error
  setTimeout(function(){
      throw new Error("Something went wrong")
  }, 100);
});

Possible solutions.

  1. Latch on to process.on("uncaughtException"). When the asynchronous exception is thrown somehow figure out what job it came from so that job can be rejected.
  2. In all async processing of a job the implementer should be diligent and make sure that resolve() or reject() is called in every possible situation.
  3. Create a new configuration for timeout_to_fail.
    • Currently, it looks like the timeout configuration re-queues the job so in this scenario the job get's requeued and then the worker hangs again.
    • This new configuration, instead of re-queueing can simply fail the job so that it doesn't get picked up again and cause another worker to hang.
    • This solution would also require re-naming the current timeout configuration to timeout_to_requeue).
  4. Any other ideas?...

2 is a good practice anyway and it should be done regardless, but there's still always a possibility of an uncaught exception happening and causing the worker to erroneously stay "busy".

How are folks dealing with this? Is there an easy solution that I'm missing?

dylanjha commented 8 years ago

updated this issue with an idea, 3. Do the maintainers have an opinion about any of these strategies? I'd happy to contribute and implement a solution to this.

meticoeus commented 8 years ago

Depending on your use-case implement your API using an async abstraction layer like Promises or RxJS Observables both of which support catching async errors and propagating them to a handler. For things like setTimeout you will need wrapper functions. Otherwise Promises for example will catch errors for you automatically

The example below was constructed to demonstrate that async errors can be caught without writing try..catch anywhere in your own code.

// Return a promise that resolves to the input after the timeout
function delayedValue(value, timeout) {
  return new Promise(function (resolve, reject) {
    setTimeout(function () {
      resolve(value)
    }, timeout)
  })
}

var queue = new Queue(ref, function(data, progress, resolve, reject) {
  // Read and process task data
  console.log(data);

  // asynchronous error
  var p = delayedValue(data, 100)
    .then(function (delayedData) {
      throw new Error("Something went wrong")
    })

  p.then(resolve, reject)
});
dylanjha commented 8 years ago

this makes sense @meticoeus, thank you