Skip to content

[Obsolete draft] Fix dropped jobs issue #197

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.

Sign up for GitHub

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

Closed
hughsw wants to merge 3 commits into from
+73 −61
Closed

[Obsolete draft] Fix dropped jobs issue #197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
623a96c
fix(lost-jobs): Fix lost-jobs issue by incrementing counter early
hughsw Apr 7, 2020
bd568e9
fix(lost-jobs): Update some tests to deal with job.id increment changes
hughsw Apr 7, 2020
5a1bb3a
feat(lost-jobs-testing): Change a removeJob test to be deterministic
hughsw Apr 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 30 additions & 16 deletions lib/job.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,34 +50,48 @@ class Job extends Emitter {
const toKey = this.queue.toKey.bind(this.queue);

let promise;
if (this.id) {
promise = Promise.resolve();
} else {
// serialize creation of value for this.id through Redis
promise = this.queue._commandable().then((client) => {
const incrPromise = helpers.deferred();
client.incr(toKey('id'), incrPromise.defer());
return incrPromise;
})
.then((value) => {
this.id = value + '';
});
}

// We add to the stored-jobs set before addJob (or addDelayedJob) so that
// this job is already in the queue's map for the rare case of the job
// getting popped off 'waiting', completing, and making callbacks (which use
// the map) all before the _evalScript promise has resolved.
if (this.queue.settings.storeJobs) {
promise = promise.then(() => this.queue.jobs.set(this.id, this));
}

if (this.options.delay) {
promise = this.queue._evalScript('addDelayedJob', 4,
toKey('id'), toKey('jobs'), toKey('delayed'), toKey('earlierDelayed'),
this.id || '', this.toData(), this.options.delay);
promise = promise.then(() => this.queue._evalScript('addDelayedJob', 3,
toKey('jobs'), toKey('delayed'), toKey('earlierDelayed'),
this.id, this.toData(), this.options.delay));

if (this.queue.settings.activateDelayedJobs) {
promise = promise.then((jobId) => {
// Only reschedule if the job was actually created.
// Only reschedule if a new job was actually created.
if (jobId) {
this.queue._delayedTimer.schedule(this.options.delay);
}
return jobId;
});
}
} else {
promise = this.queue._evalScript('addJob', 3,
toKey('id'), toKey('jobs'), toKey('waiting'),
this.id || '', this.toData());
promise = promise.then(() => this.queue._evalScript('addJob', 2,
toKey('jobs'), toKey('waiting'),
this.id, this.toData()));
}

promise = promise.then((jobId) => {
this.id = jobId;
// If the jobId is not null, then store the job in the job map.
if (jobId && this.queue.settings.storeJobs) {
this.queue.jobs.set(jobId, this);
}
return this;
});
promise = promise.then(() => this);

if (cb) helpers.asCallback(promise, cb);
return promise;
Expand Down
20 changes: 8 additions & 12 deletions lib/lua/addDelayedJob.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,24 @@
--[[
key 1 -> bq:name:id (job ID counter)
key 2 -> bq:name:jobs
key 3 -> bq:name:delayed
key 4 -> bq:name:earlierDelayed
key 1 -> bq:name:jobs
key 2 -> bq:name:delayed
key 3 -> bq:name:earlierDelayed
arg 1 -> job id
arg 2 -> job data
arg 3 -> job delay timestamp
]]

local jobId = ARGV[1]
if jobId == "" then
jobId = "" .. redis.call("incr", KEYS[1])
end
if redis.call("hexists", KEYS[2], jobId) == 1 then return nil end
redis.call("hset", KEYS[2], jobId, ARGV[2])
redis.call("zadd", KEYS[3], tonumber(ARGV[3]), jobId)
if redis.call("hexists", KEYS[1], jobId) == 1 then return nil end
redis.call("hset", KEYS[1], jobId, ARGV[2])
redis.call("zadd", KEYS[2], tonumber(ARGV[3]), jobId)

-- if this job is the new head, alert the workers that they need to update their timers
-- if we try to do something tricky like checking the delta between this job and the next job, we
-- can enter a pathological case where jobs incrementally creep sooner, and each one never updates
-- the timers
local head = redis.call("zrange", KEYS[3], 0, 0)
local head = redis.call("zrange", KEYS[2], 0, 0)
if head[1] == jobId then
redis.call("publish", KEYS[4], ARGV[3])
redis.call("publish", KEYS[3], ARGV[3])
end

return jobId
16 changes: 5 additions & 11 deletions lib/lua/addJob.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,11 @@
--[[
key 1 -> bq:name:id (job ID counter)
key 2 -> bq:name:jobs
key 3 -> bq:name:waiting
key 1 -> bq:name:jobs
key 2 -> bq:name:waiting
arg 1 -> job id
arg 2 -> job data
]]

local jobId = ARGV[1]
if jobId == "" then
jobId = "" .. redis.call("incr", KEYS[1])
end
if redis.call("hexists", KEYS[2], jobId) == 1 then return nil end
redis.call("hset", KEYS[2], jobId, ARGV[2])
redis.call("lpush", KEYS[3], jobId)

return jobId
if redis.call("hexists", KEYS[1], jobId) == 1 then return end
redis.call("hset", KEYS[1], jobId, ARGV[2])
redis.call("lpush", KEYS[2], jobId)
14 changes: 12 additions & 2 deletions lib/queue.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,11 +420,20 @@ class Queue extends Emitter {
return promise;
}

_waitForJob() {
const idPromise = helpers.deferred();
_waitForJob(testingCb) {
let idPromise = helpers.deferred();
this.bclient.brpoplpush(this.toKey('waiting'), this.toKey('active'), 0,
idPromise.defer());

// The only reason for this callback is to support deterministic testing of
// removeJob between the time brpoplpush resolves and Job.fromId() runs
Comment on lines +428 to +429
Copy link
Member

@skeggse skeggse Apr 8, 2020

Choose a reason for hiding this comment

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

Instead of modifying the source to support the test, what do you think about modifying the test to either stub the Job.fromId static method or modify the brpoplpush function to provide this hook?

Copy link
Collaborator Author

@hughsw hughsw Apr 15, 2020

Choose a reason for hiding this comment

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

Yes, I had thought about wrapping Job.fromId() -- I'll revisit that approach.

if (testingCb) {
idPromise = idPromise.then(async (jobId) => {
await testingCb(jobId);
return jobId;
});
}

return idPromise.then((jobId) => {
// Note that the job may be null in the case that the client has removed
// the job before processing can take place, but after the brpoplpush has
Expand Down Expand Up @@ -725,6 +734,7 @@ class Queue extends Emitter {
.then((results) => {
const numRaised = results[0], nextOpportunity = results[1];
if (numRaised) {
// not documented
Copy link
Member

@skeggse skeggse Apr 8, 2020

Choose a reason for hiding this comment

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

heh yeah

This seems unrelated to the overarching PR, and maybe we could document it instead of leaving it more explicitly undocumented.

this.emit('raised jobs', numRaised);
}
this._delayedTimer.schedule(parseInt(nextOpportunity, 10));
Expand Down
38 changes: 18 additions & 20 deletions test/queue-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ describe('Queue', (it) => {
const jobs = spitter();
queue.process((job) => jobs.pushSuspend(job));

await queue.createJob({}).save();
const job = await queue.createJob({}).save();
const [, finishJob] = await jobs.shift();

await t.throws(queue.close(10));
Expand All @@ -352,7 +352,7 @@ describe('Queue', (it) => {

const errors = t.context.queueErrors, count = errors.length;
t.context.queueErrors = errors.filter((err) => {
return err.message !== 'unable to update the status of succeeded job 1';
return err.message !== `unable to update the status of succeeded job ${job.id}`;
});
t.is(t.context.queueErrors.length, count - 1);
t.context.handleErrors(t);
Expand Down Expand Up @@ -460,12 +460,12 @@ describe('Queue', (it) => {
// Override _waitForJob.
const waitJob = queue._waitForJob, wait = helpers.deferred();
let waitDone = wait.defer();
queue._waitForJob = function (...args) {
queue._waitForJob = function () {
if (waitDone) {
waitDone();
waitDone = null;
}
return waitJob.apply(this, args);
return waitJob.apply(this);
Copy link
Member

@skeggse skeggse Apr 8, 2020

Choose a reason for hiding this comment

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

apply like this just always feels weird to me :\

Suggested change
return waitJob.apply(this);
return waitJob.call(this);

Copy link
Collaborator Author

@hughsw hughsw Apr 15, 2020

Choose a reason for hiding this comment

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

You're right.

};

await wait;
Expand Down Expand Up @@ -878,33 +878,31 @@ describe('Queue', (it) => {
});

it.describe('removeJob', (it) => {
it('should not cause an error if immediately removed', async (t) => {
it('should not error if active then removed before running', async (t) => {
const queue = t.context.makeQueue();

queue.process(async (job) => {
if (job.id === 'deadjob') {
t.fail('should not be able to process the job');
}
});

// Inject removeJob() inside _waitForJob the first time it's called
const waitJob = queue._waitForJob, wait = helpers.deferred();
let waitDone = wait.defer();
queue._waitForJob = function (...args) {
queue._waitForJob = function () {
const args = [];
if (waitDone) {
args.push((jobId) => this.removeJob(jobId));
waitDone();
waitDone = null;
}
return waitJob.apply(this, args);
};

await wait;
queue.process(async (job) => {
if (job.id === 'deadjob') {
t.fail('should not be able to process the job');
}
});

const job = queue.createJob({foo: 'bar'}).setId('deadjob');
await Promise.all([
job.save(),
queue.removeJob(job.id),
queue.createJob({foo: 'bar'}).setId('goodjob').save(),
]);
await wait;
await queue.createJob({foo: 'bar'}).setId('deadjob').save();
await queue.createJob({foo: 'bar'}).setId('goodjob').save();

const goodJob = await helpers.waitOn(queue, 'succeeded');
t.is(goodJob.id, 'goodjob');
Expand Down Expand Up @@ -1163,7 +1161,7 @@ describe('Queue', (it) => {
const [[failedJob, err]] = fail.args;

t.truthy(failedJob);
t.is(job.id, '1');
t.is(job.id, failedJob.id);
t.is(failedJob.data.foo, 'bar');
t.is(err.message, `Job ${job.id} timed out (10 ms)`);
});
Expand Down