Skip to content

Commit a0e4125

Browse files
committed
Ensure scheduleOnce works correctly following a cancelled job
Previously, cancelling a job would `splice` it from the queue. This was problematic because it affects the index of all later jobs in the queue. The `targetQueues` maintains of jobs to queue indexes becomes incorrect, leading to arguments being passed to the wrong jobs. For example, given the sequence: ```javascript const toCancel = bb.scheduleOnce('queueName', null, f1, "f1 cancelled schedule"); bb.scheduleOnce('queueName', null, f2, "f2 first schedule"); bb.scheduleOnce('queueName', null, f3, "f3 first schedule"); bb.cancel(toCancel); bb.scheduleOnce('queueName', null, f2, "f2 second schedule"); ``` The correct behaviour is for `f2` to be called with the argument `"f2 second schedule"`, and f3 to be called with `"f3 first schedule"`. In reality, `f2` was being called with the the argument from its first schedule ("f2 first schedule") and f3 was being called with the argument from f2's second schedule ("f2 second schedule"). One possible fix would be to iterate over all `targetQueues` and decrement them by `QUEUE_ITEM_LENGTH` if they are greater than the cancelled job's index. That would be `O(n)` where `n` is the number of queued jobs. Instead, this commit nulls out out 'method' of the cancelled job in the queue, thereby making it a no-op (`O(1)`). That is the same technique employed a few lines below when cancelling the job in `_queueBeingFlushed`.
1 parent af77b18 commit a0e4125

File tree

2 files changed

+27
-1
lines changed

2 files changed

+27
-1
lines changed

lib/backburner/queue.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ export default class Queue {
121121
let index = findItem(target, method, queue);
122122

123123
if (index > -1) {
124-
queue.splice(index, QUEUE_ITEM_LENGTH);
124+
queue[index + 1] = null;
125125
return true;
126126
}
127127

tests/cancel-test.ts

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,32 @@ QUnit.test('scheduleOnce', function(assert) {
1818
});
1919
});
2020

21+
QUnit.test('cancelling does not affect future scheduleOnce calls', function(assert) {
22+
assert.expect(5);
23+
24+
let bb = new Backburner(['queueName']);
25+
const f1Calls: string[] = [];
26+
const f2Calls: string[] = [];
27+
const f3Calls: string[] = [];
28+
const f1 = (arg: string) => f1Calls.push(arg);
29+
const f2 = (arg: string) => f2Calls.push(arg);
30+
const f3 = (arg: string) => f3Calls.push(arg);
31+
32+
bb.run(() => {
33+
const toCancel = bb.scheduleOnce('queueName', null, f1, "f1 cancelled schedule");
34+
bb.scheduleOnce('queueName', null, f2, "f2 first schedule");
35+
bb.scheduleOnce('queueName', null, f3, "f3 first schedule");
36+
bb.cancel(toCancel);
37+
bb.scheduleOnce('queueName', null, f2, "f2 second schedule");
38+
});
39+
40+
assert.equal(f1Calls.length, 0, "f1 was not called")
41+
assert.equal(f2Calls.length, 1, "f2 was called once")
42+
assert.equal(f3Calls.length, 1, "f3 was called once")
43+
assert.deepEqual(f2Calls, ["f2 second schedule"], "f2 received the correct argument")
44+
assert.deepEqual(f3Calls, ["f3 first schedule"], "f3 received the correct argument")
45+
});
46+
2147
QUnit.test('setTimeout', function(assert) {
2248
assert.expect(5);
2349
let done = assert.async();

0 commit comments

Comments
 (0)