Skip to content

Commit 11d6606

Browse files
simllllsimllll
committed
fix(jobprocessor): ensure returnNextConcurrencyFreeJob is not returning same job each time
1 parent a3d4203 commit 11d6606

File tree

2 files changed

+111
-110
lines changed

2 files changed

+111
-110
lines changed

‎src/JobProcessingQueue.ts

+11-7
Original file line numberDiff line numberDiff line change
@@ -91,13 +91,16 @@ export class JobProcessingQueue {
9191
* @param {Object} jobStatus current status of jobs
9292
* @returns {Job} Next Job to be processed
9393
*/
94-
returnNextConcurrencyFreeJob(jobStatus: {
95-
[jobName: string]:
96-
| {
97-
running: number;
98-
}
99-
| undefined;
100-
}): (Job & { attrs: IJobParameters & { nextRunAt: Date } }) | undefined {
94+
returnNextConcurrencyFreeJob(
95+
jobStatus: {
96+
[jobName: string]:
97+
| {
98+
running: number;
99+
}
100+
| undefined;
101+
},
102+
handledJobs: IJobParameters['_id'][]
103+
): (Job & { attrs: IJobParameters & { nextRunAt: Date } }) | undefined {
101104
const next = ((Object.keys(this._queue) as unknown) as number[]).reverse().find(i => {
102105
const def = this.agenda.definitions[this._queue[i].attrs.name];
103106
const status = jobStatus[this._queue[i].attrs.name];
@@ -109,6 +112,7 @@ export class JobProcessingQueue {
109112
if (
110113
def &&
111114
this._queue[i].attrs.nextRunAt &&
115+
!handledJobs.includes(this._queue[i].attrs._id) &&
112116
(!status || !def.concurrency || status.running < def.concurrency)
113117
) {
114118
return true;

‎src/JobProcessor.ts

+100-103
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { IAgendaStatus } from './types/AgendaStatus';
44
import { IJobDefinition } from './types/JobDefinition';
55
import { JobProcessingQueue } from './JobProcessingQueue';
66
import type { Agenda } from './index';
7+
import type { IJobParameters } from './types/JobParameters';
78

89
const log = debug('agenda:jobProcessor');
910

@@ -333,7 +334,6 @@ export class JobProcessor {
333334

334335
this.enqueueJob(job);
335336
await this.jobQueueFilling(name);
336-
// this.jobProcessing();
337337
} else {
338338
log.extend('jobQueueFilling')('Cannot lock job [%s]', name);
339339
}
@@ -344,22 +344,22 @@ export class JobProcessor {
344344

345345
/**
346346
* Internal method that processes any jobs in the local queue (array)
347+
* handledJobs keeps list of already processed jobs
347348
* @returns {undefined}
348349
*/
349-
private async jobProcessing() {
350+
private async jobProcessing(handledJobs: IJobParameters['_id'][] = []) {
350351
// Ensure we have jobs
351352
if (this.jobQueue.length === 0) {
352353
return;
353354
}
354355

355356
this.localQueueProcessing += 1;
356357

357-
let jobEnqueued = false;
358358
try {
359359
const now = new Date();
360360

361361
// Check if there is any job that is not blocked by concurrency
362-
const job = this.jobQueue.returnNextConcurrencyFreeJob(this.jobStatus);
362+
const job = this.jobQueue.returnNextConcurrencyFreeJob(this.jobStatus, handledJobs);
363363

364364
if (!job) {
365365
log.extend('jobProcessing')('[%s:%s] there is no job to process');
@@ -380,7 +380,7 @@ export class JobProcessor {
380380
job.attrs.name,
381381
job.attrs._id
382382
);
383-
jobEnqueued = await this.runOrRetry(job);
383+
this.runOrRetry(job);
384384
} else {
385385
const runIn = job.attrs.nextRunAt.getTime() - now.getTime();
386386
log.extend('jobProcessing')(
@@ -392,11 +392,12 @@ export class JobProcessor {
392392
setTimeout(() => {
393393
this.jobProcessing();
394394
}, runIn);
395-
jobEnqueued = true;
396395
}
397-
if (this.localQueueProcessing < this.maxConcurrency && jobEnqueued) {
396+
397+
if (this.localQueueProcessing < this.maxConcurrency) {
398398
// additionally run again and check if there are more jobs that we can process right now (as long concurrency not reached)
399-
setImmediate(() => this.jobProcessing());
399+
handledJobs.push(job.attrs._id);
400+
setImmediate(() => this.jobProcessing(handledJobs));
400401
}
401402
} finally {
402403
this.localQueueProcessing -= 1;
@@ -407,15 +408,15 @@ export class JobProcessor {
407408
* Internal method that tries to run a job and if it fails, retries again!
408409
* @returns {boolean} processed a job or not
409410
*/
410-
private async runOrRetry(job: Job): Promise<boolean> {
411+
private async runOrRetry(job: Job): Promise<void> {
411412
if (!this.isRunning) {
412413
// const a = new Error();
413414
// console.log('STACK', a.stack);
414415
log.extend('runOrRetry')(
415416
'JobProcessor got stopped already while calling runOrRetry, returning!',
416417
this
417418
);
418-
return false;
419+
return;
419420
}
420421

421422
this.jobQueue.remove(job);
@@ -448,109 +449,106 @@ export class JobProcessor {
448449

449450
this.lockedJobs.splice(lockedJobIndex, 1);
450451
this.updateStatus(job.attrs.name, 'locked', -1);
451-
return true;
452+
return;
452453
}
453454

454-
const runJob = async () => {
455-
// Add to local "running" queue
456-
this.runningJobs.push(job);
457-
this.updateStatus(job.attrs.name, 'running', 1);
458-
459-
try {
460-
log.extend('runOrRetry')('[%s:%s] processing job', job.attrs.name, job.attrs._id);
461-
462-
// check if the job is still alive
463-
const checkIfJobIsStillAlive = () => {
464-
// check every "this.agenda.definitions[job.attrs.name].lockLifetime / 2"" (or at mininum every processEvery)
465-
return new Promise((resolve, reject) =>
466-
setTimeout(() => {
467-
// when job is not running anymore, just finish
468-
if (!job.isRunning()) {
469-
resolve();
470-
return;
471-
}
472-
473-
if (job.isDead()) {
474-
reject(
475-
new Error(
476-
`execution of '${job.attrs.name}' canceled, execution took more than ${
477-
this.agenda.definitions[job.attrs.name].lockLifetime
478-
}ms. Call touch() for long running jobs to keep them alive.`
479-
)
480-
);
481-
return;
482-
}
483-
484-
resolve(checkIfJobIsStillAlive());
485-
}, Math.max(this.processEvery, this.agenda.definitions[job.attrs.name].lockLifetime / 2))
486-
);
487-
};
455+
// Add to local "running" queue
456+
this.runningJobs.push(job);
457+
this.updateStatus(job.attrs.name, 'running', 1);
458+
459+
try {
460+
log.extend('runOrRetry')('[%s:%s] processing job', job.attrs.name, job.attrs._id);
461+
462+
// check if the job is still alive
463+
const checkIfJobIsStillAlive = () => {
464+
// check every "this.agenda.definitions[job.attrs.name].lockLifetime / 2"" (or at mininum every processEvery)
465+
return new Promise((resolve, reject) =>
466+
setTimeout(() => {
467+
// when job is not running anymore, just finish
468+
if (!job.isRunning()) {
469+
resolve();
470+
return;
471+
}
472+
473+
if (job.isDead()) {
474+
reject(
475+
new Error(
476+
`execution of '${job.attrs.name}' canceled, execution took more than ${
477+
this.agenda.definitions[job.attrs.name].lockLifetime
478+
}ms. Call touch() for long running jobs to keep them alive.`
479+
)
480+
);
481+
return;
482+
}
483+
484+
resolve(checkIfJobIsStillAlive());
485+
}, Math.max(this.processEvery, this.agenda.definitions[job.attrs.name].lockLifetime / 2))
486+
);
487+
};
488+
489+
// CALL THE ACTUAL METHOD TO PROCESS THE JOB!!!
490+
await Promise.race([job.run(), checkIfJobIsStillAlive()]);
488491

489-
// CALL THE ACTUAL METHOD TO PROCESS THE JOB!!!
490-
await Promise.race([job.run(), checkIfJobIsStillAlive()]);
492+
log.extend('runOrRetry')(
493+
'[%s:%s] processing job successfull',
494+
job.attrs.name,
495+
job.attrs._id
496+
);
491497

498+
// Job isn't in running jobs so throw an error
499+
if (!this.runningJobs.includes(job)) {
492500
log.extend('runOrRetry')(
493-
'[%s:%s] processing job successfull',
494-
job.attrs.name,
501+
'[%s] callback was called, job must have been marked as complete already',
495502
job.attrs._id
496503
);
504+
throw new Error(
505+
`callback already called - job ${job.attrs.name} already marked complete`
506+
);
507+
}
508+
} catch (err) {
509+
job.canceled = err;
510+
log.extend('runOrRetry')(
511+
'[%s:%s] processing job failed',
512+
job.attrs.name,
513+
job.attrs._id,
514+
err
515+
);
516+
job.agenda.emit('error', err);
517+
} finally {
518+
// Remove the job from the running queue
519+
let runningJobIndex = this.runningJobs.indexOf(job);
520+
if (runningJobIndex === -1) {
521+
// lookup by id
522+
runningJobIndex = this.runningJobs.findIndex(
523+
j => j.attrs._id?.toString() === job.attrs._id?.toString()
524+
);
525+
}
526+
if (runningJobIndex === -1) {
527+
// eslint-disable-next-line no-unsafe-finally
528+
throw new Error(`cannot find job ${job.attrs._id} in running jobs queue?`);
529+
}
530+
this.runningJobs.splice(runningJobIndex, 1);
531+
this.updateStatus(job.attrs.name, 'running', -1);
497532

498-
// Job isn't in running jobs so throw an error
499-
if (!this.runningJobs.includes(job)) {
500-
log.extend('runOrRetry')(
501-
'[%s] callback was called, job must have been marked as complete already',
502-
job.attrs._id
503-
);
504-
throw new Error(
505-
`callback already called - job ${job.attrs.name} already marked complete`
506-
);
507-
}
508-
} catch (err) {
509-
job.canceled = err;
510-
log.extend('runOrRetry')(
511-
'[%s:%s] processing job failed',
512-
job.attrs.name,
513-
job.attrs._id,
514-
err
533+
// Remove the job from the locked queue
534+
let lockedJobIndex = this.lockedJobs.indexOf(job);
535+
if (lockedJobIndex === -1) {
536+
// lookup by id
537+
lockedJobIndex = this.lockedJobs.findIndex(
538+
j => j.attrs._id?.toString() === job.attrs._id?.toString()
515539
);
516-
job.agenda.emit('error', err);
517-
} finally {
518-
// Remove the job from the running queue
519-
let runningJobIndex = this.runningJobs.indexOf(job);
520-
if (runningJobIndex === -1) {
521-
// lookup by id
522-
runningJobIndex = this.runningJobs.findIndex(
523-
j => j.attrs._id?.toString() === job.attrs._id?.toString()
524-
);
525-
}
526-
if (runningJobIndex === -1) {
527-
// eslint-disable-next-line no-unsafe-finally
528-
throw new Error(`cannot find job ${job.attrs._id} in running jobs queue?`);
529-
}
530-
this.runningJobs.splice(runningJobIndex, 1);
531-
this.updateStatus(job.attrs.name, 'running', -1);
532-
533-
// Remove the job from the locked queue
534-
let lockedJobIndex = this.lockedJobs.indexOf(job);
535-
if (lockedJobIndex === -1) {
536-
// lookup by id
537-
lockedJobIndex = this.lockedJobs.findIndex(
538-
j => j.attrs._id?.toString() === job.attrs._id?.toString()
539-
);
540-
}
541-
if (lockedJobIndex === -1) {
542-
// eslint-disable-next-line no-unsafe-finally
543-
throw new Error(`cannot find job ${job.attrs._id} in locked jobs queue?`);
544-
}
545-
this.lockedJobs.splice(lockedJobIndex, 1);
546-
this.updateStatus(job.attrs.name, 'locked', -1);
547540
}
541+
if (lockedJobIndex === -1) {
542+
// eslint-disable-next-line no-unsafe-finally
543+
throw new Error(`cannot find job ${job.attrs._id} in locked jobs queue?`);
544+
}
545+
this.lockedJobs.splice(lockedJobIndex, 1);
546+
this.updateStatus(job.attrs.name, 'locked', -1);
547+
}
548548

549-
// Re-process jobs now that one has finished
550-
setImmediate(() => this.jobProcessing());
551-
};
552-
runJob();
553-
return true;
549+
// Re-process jobs now that one has finished
550+
setImmediate(() => this.jobProcessing());
551+
return;
554552
}
555553

556554
// Run the job later
@@ -560,7 +558,6 @@ export class JobProcessor {
560558
job.attrs._id
561559
);
562560
this.enqueueJob(job);
563-
return false;
564561
}
565562

566563
private updateStatus(name: string, key: 'locked' | 'running', number: -1 | 1) {

0 commit comments

Comments
 (0)