Skip to content

Commit b365957

Browse files
simllllsimllll
committed
fix: job timeout check and improve error handling for childs
1 parent 46384bd commit b365957

File tree

3 files changed

+76
-80
lines changed

3 files changed

+76
-80
lines changed

‎src/Job.ts

+61-67
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import * as date from 'date.js';
22
import * as debug from 'debug';
33
import { ObjectId } from 'mongodb';
4-
import { fork } from 'child_process';
4+
import { ChildProcess, fork } from 'child_process';
55
import type { Agenda } from './index';
66
import type { DefinitionProcessor } from './types/JobDefinition';
77
import { IJobParameters, datefields, TJobDatefield } from './types/JobParameters';
@@ -20,7 +20,27 @@ export class Job<DATA = unknown | void> {
2020
* you can use it for long running tasks to periodically check if canceled is true,
2121
* also touch will check if and throws that the job got canceled
2222
*/
23-
canceled: Error | undefined;
23+
private canceled?: Error | true;
24+
25+
getCanceledMessage() {
26+
return typeof this.canceled === 'object'
27+
? this.canceled?.message || this.canceled
28+
: this.canceled;
29+
}
30+
31+
private forkedChild?: ChildProcess;
32+
33+
cancel(error?: Error) {
34+
this.canceled = error || true;
35+
if (this.forkedChild) {
36+
try {
37+
this.forkedChild.send('cancel');
38+
console.info('canceled child', this.attrs.name, this.attrs._id);
39+
} catch (err) {
40+
console.log('cannot send cancel to child');
41+
}
42+
}
43+
}
2444

2545
/** internal variable to ensure a job does not set unlimited numbers of setTimeouts if the job is not processed
2646
* immediately */
@@ -266,12 +286,6 @@ export class Job<DATA = unknown | void> {
266286
}
267287

268288
async isDead(): Promise<boolean> {
269-
if (!this.byJobProcessor || this.attrs.fork) {
270-
// we have no job definition, therfore we are not the job processor, but a client call
271-
// so we get the real state from database
272-
await this.fetchStatus();
273-
}
274-
275289
return this.isExpired();
276290
}
277291

@@ -360,68 +374,47 @@ export class Job<DATA = unknown | void> {
360374
}
361375
const { forkHelper } = this.agenda;
362376

363-
let stillRunning = true;
364-
try {
365-
await new Promise<void>((resolve, reject) => {
366-
const child = fork(
367-
forkHelper.path,
368-
[
377+
await new Promise<void>((resolve, reject) => {
378+
this.forkedChild = fork(
379+
forkHelper.path,
380+
[
381+
this.attrs.name,
382+
this.attrs._id!.toString(),
383+
this.agenda.definitions[this.attrs.name].filePath || ''
384+
],
385+
forkHelper.options
386+
);
387+
388+
let childError: any;
389+
this.forkedChild.on('close', code => {
390+
if (code) {
391+
console.info(
392+
'fork parameters',
393+
forkHelper,
369394
this.attrs.name,
370-
this.attrs._id!.toString(),
371-
this.agenda.definitions[this.attrs.name].filePath || ''
372-
],
373-
forkHelper.options
374-
);
375-
376-
child.on('close', code => {
377-
stillRunning = false;
378-
if (code) {
379-
console.info(
380-
'fork parameters',
381-
forkHelper,
382-
this.attrs.name,
383-
this.attrs._id,
384-
this.agenda.definitions[this.attrs.name].filePath
385-
);
386-
const error = new Error(`child process exited with code: ${code}`);
387-
console.warn(error.message);
388-
reject(error);
389-
} else {
390-
resolve();
391-
}
392-
});
393-
child.on('message', message => {
394-
// console.log(`Message from child.js: ${message}`, JSON.stringify(message));
395-
if (typeof message === 'string') {
396-
try {
397-
reject(JSON.parse(message));
398-
} catch (errJson) {
399-
reject(message);
400-
}
401-
} else {
402-
reject(message);
395+
this.attrs._id,
396+
this.agenda.definitions[this.attrs.name].filePath
397+
);
398+
const error = new Error(`child process exited with code: ${code}`);
399+
console.warn(error.message);
400+
reject(childError || error);
401+
} else {
402+
resolve();
403+
}
404+
});
405+
this.forkedChild.on('message', message => {
406+
// console.log(`Message from child.js: ${message}`, JSON.stringify(message));
407+
if (typeof message === 'string') {
408+
try {
409+
childError = JSON.parse(message);
410+
} catch (errJson) {
411+
childError = message;
403412
}
404-
});
405-
406-
// check if job is still alive
407-
const checkCancel = () =>
408-
setTimeout(() => {
409-
if (this.canceled) {
410-
try {
411-
child.send('cancel');
412-
console.info('canceled child', this.attrs.name, this.attrs._id);
413-
} catch (err) {
414-
console.log('cannot send cancel to child');
415-
}
416-
} else if (stillRunning) {
417-
setTimeout(checkCancel, 10000);
418-
}
419-
});
420-
checkCancel();
413+
} else {
414+
childError = message;
415+
}
421416
});
422-
} finally {
423-
stillRunning = false;
424-
}
417+
});
425418
} else {
426419
await this.runJob();
427420
}
@@ -440,6 +433,7 @@ export class Job<DATA = unknown | void> {
440433
this.agenda.emit(`fail:${this.attrs.name}`, error, this);
441434
log('[%s:%s] has failed [%s]', this.attrs.name, this.attrs._id, error.message);
442435
} finally {
436+
this.forkedChild = undefined;
443437
this.attrs.lockedAt = undefined;
444438
try {
445439
await this.agenda.db.saveJobState(this);

‎src/JobProcessor.ts

+9-7
Original file line numberDiff line numberDiff line change
@@ -51,25 +51,25 @@ export class JobProcessor {
5151
? this.jobQueue.length
5252
: this.jobQueue.getQueue().map(job => ({
5353
...job.toJson(),
54-
canceled: job.canceled?.message || job.canceled
54+
canceled: job.getCanceledMessage()
5555
})),
5656
runningJobs: !fullDetails
5757
? this.runningJobs.length
5858
: this.runningJobs.map(job => ({
5959
...job.toJson(),
60-
canceled: job.canceled?.message || job.canceled
60+
canceled: job.getCanceledMessage()
6161
})),
6262
lockedJobs: !fullDetails
6363
? this.lockedJobs.length
6464
: this.lockedJobs.map(job => ({
6565
...job.toJson(),
66-
canceled: job.canceled?.message || job.canceled
66+
canceled: job.getCanceledMessage()
6767
})),
6868
jobsToLock: !fullDetails
6969
? this.jobsToLock.length
7070
: this.jobsToLock.map(job => ({
7171
...job.toJson(),
72-
canceled: job.canceled?.message || job.canceled
72+
canceled: job.getCanceledMessage()
7373
})),
7474
isLockingOnTheFly: this.isLockingOnTheFly
7575
};
@@ -499,6 +499,7 @@ export class JobProcessor {
499499
this.runningJobs.push(job);
500500
this.updateStatus(job.attrs.name, 'running', 1);
501501

502+
let jobIsRunning = true;
502503
try {
503504
log.extend('runOrRetry')('[%s:%s] processing job', job.attrs.name, job.attrs._id);
504505

@@ -508,7 +509,7 @@ export class JobProcessor {
508509
new Promise<void>((resolve, reject) => {
509510
setTimeout(async () => {
510511
// when job is not running anymore, just finish
511-
if (!(await job.isRunning())) {
512+
if (!jobIsRunning) {
512513
resolve();
513514
return;
514515
}
@@ -547,8 +548,7 @@ export class JobProcessor {
547548
);
548549
}
549550
} catch (error: any) {
550-
// eslint-disable-next-line no-param-reassign
551-
job.canceled = error;
551+
job.cancel(error);
552552
log.extend('runOrRetry')(
553553
'[%s:%s] processing job failed',
554554
job.attrs.name,
@@ -557,6 +557,8 @@ export class JobProcessor {
557557
);
558558
this.agenda.emit('error', error);
559559
} finally {
560+
jobIsRunning = false;
561+
560562
// Remove the job from the running queue
561563
let runningJobIndex = this.runningJobs.indexOf(job);
562564
if (runningJobIndex === -1) {

‎src/index.ts

+6-6
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import { JobDbRepository } from './JobDbRepository';
1414
import { JobPriority, parsePriority } from './utils/priority';
1515
import { JobProcessor } from './JobProcessor';
1616
import { calculateProcessEvery } from './utils/processEvery';
17-
import {getCallerFilePath} from "./utils/stack";
17+
import { getCallerFilePath } from './utils/stack';
1818

1919
const log = debug('agenda');
2020

@@ -339,11 +339,11 @@ export class Agenda extends EventEmitter {
339339
log('overwriting already defined agenda job', name);
340340
}
341341

342-
const filePath = getCallerFilePath();
342+
const filePath = getCallerFilePath();
343343

344-
this.definitions[name] = {
344+
this.definitions[name] = {
345345
fn: processor,
346-
filePath,
346+
filePath,
347347
concurrency: options?.concurrency || this.attrs.defaultConcurrency,
348348
lockLimit: options?.lockLimit || this.attrs.defaultLockLimit,
349349
priority: parsePriority(options?.priority),
@@ -538,7 +538,7 @@ export class Agenda extends EventEmitter {
538538
this.attrs.processEvery
539539
);
540540

541-
this.on('processJob', job => this.jobProcessor?.process(job));
541+
this.on('processJob', this.jobProcessor.process);
542542
}
543543

544544
/**
@@ -552,7 +552,7 @@ export class Agenda extends EventEmitter {
552552

553553
log('Agenda.stop called, clearing interval for processJobs()');
554554

555-
const lockedJobs = this.jobProcessor?.stop();
555+
const lockedJobs = this.jobProcessor.stop();
556556

557557
log('Agenda._unlockJobs()');
558558
const jobIds = lockedJobs?.map(job => job.attrs._id) || [];

0 commit comments

Comments
 (0)