taskforcesh / bullmq

BullMQ - Message Queue and Batch processing for NodeJS and Python based on Redis
https://bullmq.io
MIT License
6.18k stars 406 forks source link

Throw custom error class is not propagated properly to the Queue #254

Closed felixmosh closed 4 years ago

felixmosh commented 4 years ago

Hi, my worker throws a custom error class, I do see the error with the relevant class name & stack trace in the Bull-Dashboard.

But when the failed event triggered on the queue process I get only the errors message field. https://github.com/taskforcesh/bullmq/blob/master/src/classes/job.ts#L375

class CustomError extends Error {
  public code: any;
  public details: string[];

  constructor(code, message, details) {
    super(message);

    Error.captureStackTrace(this, this.constructor);

    this.name = this.constructor.name;
    this.message = message;

    this.code = code;
    this.details = details;
  }
}

// worker - a separate process
const worker = new Worker(
  'queueName',
  async (job) => throw new CustomError(4004, 'Message not found', 'more details')
);

// queue - main process
queue = new Queue('queueName');
const job = await queue.add('test', { foo: 'bar' });
try {
  const result = await job.waitUntilFinished(queueEvents);
} catch (error) {
  // error.message will contain "Message not found"
  // here the error doesn't have any fields of the custom error event.  (such as name, code)
}

In bull-dashboard I do see the original error object, that means that the info exists in redis. image

I hope that the issue is clear, if not, I'm available for more details.

mattwynne commented 4 years ago

I guess this is happening because the error is going through a serialization/deserialization process as it comes back through Redis.

I'm also having issues with this as I'd like to be able to raise custom errors in the workers. I'd like a way to be able to hook into the serialization/deserialization so I can control the Error types being returned from the job promise.

felixmosh commented 4 years ago

This is the place which copies the message property https://github.com/taskforcesh/bullmq/blob/cef134f7c4d87e1b80ba42a5e06c3877956ff4cc/src/classes/job.ts#L254

manast commented 4 years ago

yeah, it is impossible to keep that class since as mentioned above the error has been serialized and deserialiized.

felixmosh commented 4 years ago

How about exposing some kind of serialize & deserialize methods, there I can make it to work.

In the serilize it will pass the class obj, there i will return a serializable PoJO. And on the other side it will invoke the deserialize method with the POJO.

WDYT?

manast commented 4 years ago

there are plans for custom serializers but this is not so simple, for instance, the UI would need to also know about the serialize you are using so that it can display the jobs data correctly.

manast commented 4 years ago

not sure if it would help in your case, but maybe simpler would be if it was possible to add some "type" field to the error that is being thrown, then you could check that for discriminate which error it is.

felixmosh commented 4 years ago

there are plans for custom serializers but this is not so simple, for instance, the UI would need to also know about the serialize you are using so that it can display the jobs data correctly.

This is not an issue because it is possible to implement it as enhancement, at the worse it will fallback to the message of the error (like it is now)

felixmosh commented 4 years ago

not sure if it would help in your case, but maybe simpler would be if it was possible to add some "type" field to the error that is being thrown, then you could check that for discriminate which error it is.

This will be helpful but without the additional info it would be useless.

mattwynne commented 4 years ago

@felixmosh FWIW here's how I've worked around it for now: https://github.com/SmartBear/git-en-boite/pull/221

Essentially, it serializes all the properties and constructor name of the custom error in the message property of a regular Error as it goes through Bull, then when it comes out the other side in the client, we deserialize it.

felixmosh commented 4 years ago

@felixmosh FWIW here's how I've worked around it for now: SmartBear/git-en-boite#221

Essentially, it serializes all the properties and constructor name of the custom error in the message property of a regular Error as it goes through Bull, then when it comes out the other side in the client, we deserialize it.

Thanks for sharing your solution, actually I’ve serialized some data in the message as well but it is hacky :/

mattwynne commented 4 years ago

It could be possible to use something like https://github.com/efossas/Javascript-Serializer within Bull to serialize the error (instead of just capturing the message and stack properties) on the Job.

Perhaps we could do this in a third property on Job so other things (like the UI) that already read the message and stack could continue to rely on them?

manast commented 4 years ago

@mattwynne I am not super positive about this idea but I may change my opinion in the future.

drone1 commented 4 years ago

@mattwynne: What is the suggested pattern for getting at the nature of a failed job? Even a simple job may fail for a variety of reasons.

mattwynne commented 4 years ago

@drone1 I don't understand your question, sorry.

What I'm suggesting is to remove the assumption that the error raised in the worker will neccesarily be of type Error and support a way to serialize / deserialize custom errror types.

Even if bull were opinionated about using a particular framework / library for custom errors, I think that would be fine.

drone1 commented 1 year ago

@mattwynne I've got a worker failing on a job. The worker's failed event callback has receiving an err value that is an object.

worker.on('failed', (job, err) => {
    // A job failed with reason `err`!
    console.log(`Job "${job.id}" failed.`)
    console.log(err)   // **prints out an object like the one noted below
})

err looks something like this:

response: { ... },
config: { ... },
code: 401,
errors: [{
    message: 'Invalid Credentials',
    domain: 'global',
    reason: 'authError',
    location: 'Authorization',
    locationType: 'header'
 }]

But now things get strange.

Note that err.errors[0].message is Invalid Credentials

In my queueEvents failed callback, failedReason is simply Invalid Credentials, and all of the other data is lost. This string (Invalid Credentials) is nowhere else to be found in the original error, so BullMQ must be getting it from errors[0].message somehow.

And yet, if I explicitly throw a fake error like this...

const message = 'Invalid Credentials'
throw { errors: [{ message }]}

...then I just get null in queueEvents .on(failed)! Is this not truly bizarre behavior?

If I explicitly throw a string instead, I also get null on the other end.

Is there a best practice for getting around this now? I can "return" an error but then the job would be marked as completed. The whole reason I'm throwing errors is so it'll fail, but I need the result.

I want my job to be able to fail and pass the full err back to queueEvents, if possible.

I'm using BullMQ v3.5.11, latest version as of today.

Thank you!

manast commented 1 year ago

You must throw an Error, not an arbitrary object.

drone1 commented 1 year ago

Thank you! I’ll try this.

Perhaps BullMq could print an error message if the user throws a non-Error object?

On Tue 31. Jan 2023 at 13:07, Manuel Astudillo @.***> wrote:

You must throw an Error, not an arbitrary object.

— Reply to this email directly, view it on GitHub https://github.com/taskforcesh/bullmq/issues/254#issuecomment-1410235677, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQIEPBZUNDSJ5T2T66LICTWVD6ADANCNFSM4P62IBRA . You are receiving this because you were mentioned.Message ID: @.***>

manast commented 1 year ago

In general, you should never throw anything that is not an Error object. There is an eslint rule for that if you want to enforce it: https://eslint.org/docs/latest/rules/no-throw-literal

drone1 commented 1 year ago

if not an error, perhaps it could be mentioned in the documentation/examples if it isn’t already. I lost a lot of time on this. Thank you.

On Tue 31. Jan 2023 at 15:05, Manuel Astudillo @.***> wrote:

In general, you should never throw anything that is not an Error object. There is an eslint rule for that if you want to enforce it: https://eslint.org/docs/latest/rules/no-throw-literal

— Reply to this email directly, view it on GitHub https://github.com/taskforcesh/bullmq/issues/254#issuecomment-1410414130, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABQIEPCC73TMHOFHOJMRQ6LWVEL2VANCNFSM4P62IBRA . You are receiving this because you were mentioned.Message ID: @.***>

kibertoad commented 7 months ago

@manast Can we maybe get a simpler solution, where if there is an extra field (e. g. details), it will get JSON.stringify and included together with message and stacktrace? And then UI would just display it as-is. Then you don't need to support any fancy serializer/deserializer logic, but instead put it on user to ensure that what is there is serializable during the usual flow.

manast commented 7 months ago

@kibertoad honestly I am confused on what this issue is all about. I can see errors perfectly in Taskforce.sh, can you give me an example on how does it look like now, and how you would like it to look like?

felixmosh commented 7 months ago

If you throw an error with additional fields, you won't be able to get them (show them).

kibertoad commented 7 months ago

@manast Imagine that you are throwing an aggregate error ValidationError that under-the-hood has an array of specific ZodError instances, e. g. within the field details.errors Currently these would not be displayed at all, you will only get a top level message "Validation failed" but not any of error fields beyond message and stacktrace

It would be great to get plain stringified version of these fields

manast commented 7 months ago

I will have to think more about it...