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

perf(worker): fetch next job on failure #2342

Merged
merged 10 commits into from
Jul 19, 2024
14 changes: 9 additions & 5 deletions src/classes/job.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import {
tryCatch,
} from '../utils';
import { Backoffs } from './backoffs';
import { Scripts } from './scripts';
import { Scripts, raw2NextJobData } from './scripts';
import { UnrecoverableError } from './errors/unrecoverable-error';
import type { QueueEvents } from './queue-events';

Expand Down Expand Up @@ -616,7 +616,7 @@ export class Job<
err: E,
token: string,
fetchNext = false,
): Promise<void> {
): Promise<void | any[]> {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀 The @returns comment still says only void is returned.

const client = await this.queue.client;
const message = err?.message;

Expand Down Expand Up @@ -693,9 +693,9 @@ export class Job<
);
}

const code = results[results.length - 1][1] as number;
if (code < 0) {
throw this.scripts.finishedErrors(code, this.id, command, 'active');
const result = results[results.length - 1][1] as number;
if (result < 0) {
throw this.scripts.finishedErrors(result, this.id, command, 'active');
}

if (finishedOn && typeof finishedOn === 'number') {
Expand All @@ -707,6 +707,10 @@ export class Job<
}

this.attemptsMade += 1;

if (Array.isArray(result)) {
return raw2NextJobData(result);
}
}

/**
Expand Down
10 changes: 8 additions & 2 deletions src/classes/worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -715,8 +715,14 @@ export class Worker<
return;
}

await job.moveToFailed(err, token);
const failed = await job.moveToFailed(err, token, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "failed" here is misleading, as this includes the next job to process. Should be called probably "result" or similar.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good to me

this.emit('failed', job, err, 'active');

if (failed) {
const [jobData, jobId, limitUntil, delayUntil] = failed || [];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

|| [] is redundant as it is already checking if failed is truthy

Copy link
Collaborator Author

@roggervalf roggervalf Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

this.updateDelays(limitUntil, delayUntil);
return this.nextJobFromJobData(jobData, jobId, token);
}
} catch (err) {
this.emit('error', <Error>err);
// It probably means that the job has lost the lock before completion
Expand All @@ -735,7 +741,7 @@ export class Worker<
const result = await this.callProcessJob(job, token);
return await handleCompleted(result);
} catch (err) {
return handleFailed(<Error>err);
return await handleFailed(<Error>err);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

await here will imply an unhandled exception if handleFailed throws an exception, is this the intent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was placed when testing, removing it now

} finally {
jobsInProgress.delete(inProgressItem);
}
Expand Down