Skip to content

Commit d0cbb4c

Browse files
apapirovskiMylesBorins
apapirovski
authored and
MylesBorins
committed
lib: expose FixedQueue internally and fix nextTick bug
A bug was introduced together with the FixedQueue implementation for process.nextTick which meant that the queue wouldn't necessarily fully clear on each run through. Fix it and abstract the data structure into an internal module that can later be used elsewhere. PR-URL: #20468 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent 73f2dab commit d0cbb4c

File tree

5 files changed

+174
-116
lines changed

5 files changed

+174
-116
lines changed

‎lib/internal/fixed_queue.js‎

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
'use strict';
2+
3+
// Currently optimal queue size, tested on V8 6.0 - 6.6. Must be power of two.
4+
const kSize = 2048;
5+
const kMask = kSize - 1;
6+
7+
// The FixedQueue is implemented as a singly-linked list of fixed-size
8+
// circular buffers. It looks something like this:
9+
//
10+
// head tail
11+
// | |
12+
// v v
13+
// +-----------+ <-----\ +-----------+ <------\ +-----------+
14+
// | [null] | \----- | next | \------- | next |
15+
// +-----------+ +-----------+ +-----------+
16+
// | item | <-- bottom | item | <-- bottom | [empty] |
17+
// | item | | item | | [empty] |
18+
// | item | | item | | [empty] |
19+
// | item | | item | | [empty] |
20+
// | item | | item | bottom --> | item |
21+
// | item | | item | | item |
22+
// | ... | | ... | | ... |
23+
// | item | | item | | item |
24+
// | item | | item | | item |
25+
// | [empty] | <-- top | item | | item |
26+
// | [empty] | | item | | item |
27+
// | [empty] | | [empty] | <-- top top --> | [empty] |
28+
// +-----------+ +-----------+ +-----------+
29+
//
30+
// Or, if there is only one circular buffer, it looks something
31+
// like either of these:
32+
//
33+
// head tail head tail
34+
// | | | |
35+
// v v v v
36+
// +-----------+ +-----------+
37+
// | [null] | | [null] |
38+
// +-----------+ +-----------+
39+
// | [empty] | | item |
40+
// | [empty] | | item |
41+
// | item | <-- bottom top --> | [empty] |
42+
// | item | | [empty] |
43+
// | [empty] | <-- top bottom --> | item |
44+
// | [empty] | | item |
45+
// +-----------+ +-----------+
46+
//
47+
// Adding a value means moving `top` forward by one, removing means
48+
// moving `bottom` forward by one. After reaching the end, the queue
49+
// wraps around.
50+
//
51+
// When `top === bottom` the current queue is empty and when
52+
// `top + 1 === bottom` it's full. This wastes a single space of storage
53+
// but allows much quicker checks.
54+
55+
const FixedCircularBuffer = class FixedCircularBuffer {
56+
constructor() {
57+
this.bottom = 0;
58+
this.top = 0;
59+
this.list = new Array(kSize);
60+
this.next = null;
61+
}
62+
63+
isEmpty() {
64+
return this.top === this.bottom;
65+
}
66+
67+
isFull() {
68+
return ((this.top + 1) & kMask) === this.bottom;
69+
}
70+
71+
push(data) {
72+
this.list[this.top] = data;
73+
this.top = (this.top + 1) & kMask;
74+
}
75+
76+
shift() {
77+
const nextItem = this.list[this.bottom];
78+
if (nextItem === undefined)
79+
return null;
80+
this.list[this.bottom] = undefined;
81+
this.bottom = (this.bottom + 1) & kMask;
82+
return nextItem;
83+
}
84+
};
85+
86+
module.exports = class FixedQueue {
87+
constructor() {
88+
this.head = this.tail = new FixedCircularBuffer();
89+
}
90+
91+
isEmpty() {
92+
return this.head.isEmpty();
93+
}
94+
95+
push(data) {
96+
if (this.head.isFull()) {
97+
// Head is full: Creates a new queue, sets the old queue's `.next` to it,
98+
// and sets it as the new main queue.
99+
this.head = this.head.next = new FixedCircularBuffer();
100+
}
101+
this.head.push(data);
102+
}
103+
104+
shift() {
105+
const { tail } = this;
106+
const next = tail.shift();
107+
if (tail.isEmpty() && tail.next !== null) {
108+
// If there is another queue, it forms the new tail.
109+
this.tail = tail.next;
110+
}
111+
return next;
112+
}
113+
};

‎lib/internal/process/next_tick.js‎

Lines changed: 8 additions & 116 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ function setupNextTick() {
1616
} = require('internal/async_hooks');
1717
const promises = require('internal/process/promises');
1818
const { ERR_INVALID_CALLBACK } = require('internal/errors').codes;
19+
const FixedQueue = require('internal/fixed_queue');
1920
const { emitPromiseRejectionWarnings } = promises;
2021

2122
// tickInfo is used so that the C++ code in src/node.cc can
@@ -31,119 +32,7 @@ function setupNextTick() {
3132
const kHasScheduled = 0;
3233
const kHasPromiseRejections = 1;
3334

34-
// Queue size for each tick array. Must be a power of two.
35-
const kQueueSize = 2048;
36-
const kQueueMask = kQueueSize - 1;
37-
38-
// The next tick queue is implemented as a singly-linked list of fixed-size
39-
// circular buffers. It looks something like this:
40-
//
41-
// head tail
42-
// | |
43-
// v v
44-
// +-----------+ <-----\ +-----------+ <------\ +-----------+
45-
// | [null] | \----- | next | \------- | next |
46-
// +-----------+ +-----------+ +-----------+
47-
// | tick | <-- bottom | tick | <-- bottom | [empty] |
48-
// | tick | | tick | | [empty] |
49-
// | tick | | tick | | [empty] |
50-
// | tick | | tick | | [empty] |
51-
// | tick | | tick | bottom --> | tick |
52-
// | tick | | tick | | tick |
53-
// | ... | | ... | | ... |
54-
// | tick | | tick | | tick |
55-
// | tick | | tick | | tick |
56-
// | [empty] | <-- top | tick | | tick |
57-
// | [empty] | | tick | | tick |
58-
// | [empty] | | tick | | tick |
59-
// +-----------+ +-----------+ <-- top top --> +-----------+
60-
//
61-
// Or, if there is only one fixed-size queue, it looks something
62-
// like either of these:
63-
//
64-
// head tail head tail
65-
// | | | |
66-
// v v v v
67-
// +-----------+ +-----------+
68-
// | [null] | | [null] |
69-
// +-----------+ +-----------+
70-
// | [empty] | | tick |
71-
// | [empty] | | tick |
72-
// | tick | <-- bottom top --> | [empty] |
73-
// | tick | | [empty] |
74-
// | [empty] | <-- top bottom --> | tick |
75-
// | [empty] | | tick |
76-
// +-----------+ +-----------+
77-
//
78-
// Adding a value means moving `top` forward by one, removing means
79-
// moving `bottom` forward by one.
80-
//
81-
// We let `bottom` and `top` wrap around, so when `top` is conceptually
82-
// pointing to the end of the list, that means that the actual value is `0`.
83-
//
84-
// In particular, when `top === bottom`, this can mean *either* that the
85-
// current queue is empty or that it is full. We can differentiate by
86-
// checking whether an entry in the queue is empty (a.k.a. `=== undefined`).
87-
88-
class FixedQueue {
89-
constructor() {
90-
this.bottom = 0;
91-
this.top = 0;
92-
this.list = new Array(kQueueSize);
93-
this.next = null;
94-
}
95-
96-
push(data) {
97-
this.list[this.top] = data;
98-
this.top = (this.top + 1) & kQueueMask;
99-
}
100-
101-
shift() {
102-
const nextItem = this.list[this.bottom];
103-
if (nextItem === undefined)
104-
return null;
105-
this.list[this.bottom] = undefined;
106-
this.bottom = (this.bottom + 1) & kQueueMask;
107-
return nextItem;
108-
}
109-
}
110-
111-
var head = new FixedQueue();
112-
var tail = head;
113-
114-
function push(data) {
115-
if (head.bottom === head.top) {
116-
// Either empty or full:
117-
if (head.list[head.top] !== undefined) {
118-
// It's full: Creates a new queue, sets the old queue's `.next` to it,
119-
// and sets it as the new main queue.
120-
head = head.next = new FixedQueue();
121-
} else {
122-
// If the head is empty, that means that it was the only fixed-sized
123-
// queue in existence.
124-
DCHECK_EQ(head.next, null);
125-
// This is the first tick object in existence, so we need to inform
126-
// the C++ side that we do want to run `_tickCallback()`.
127-
tickInfo[kHasScheduled] = 1;
128-
}
129-
}
130-
head.push(data);
131-
}
132-
133-
function shift() {
134-
const next = tail.shift();
135-
if (tail.top === tail.bottom) { // -> .shift() emptied the current queue.
136-
if (tail.next !== null) {
137-
// If there is another queue, it forms the new tail.
138-
tail = tail.next;
139-
} else {
140-
// We've just run out of items. Let the native side know that it
141-
// doesn't need to bother calling into JS to run the queue.
142-
tickInfo[kHasScheduled] = 0;
143-
}
144-
}
145-
return next;
146-
}
35+
const queue = new FixedQueue();
14736

14837
process.nextTick = nextTick;
14938
// Needs to be accessible from beyond this scope.
@@ -152,7 +41,7 @@ function setupNextTick() {
15241
function _tickCallback() {
15342
let tock;
15443
do {
155-
while (tock = shift()) {
44+
while (tock = queue.shift()) {
15645
const asyncId = tock[async_id_symbol];
15746
emitBefore(asyncId, tock[trigger_async_id_symbol]);
15847
// emitDestroy() places the async_id_symbol into an asynchronous queue
@@ -175,8 +64,9 @@ function setupNextTick() {
17564

17665
emitAfter(asyncId);
17766
}
67+
tickInfo[kHasScheduled] = 0;
17868
runMicrotasks();
179-
} while (head.top !== head.bottom || emitPromiseRejectionWarnings());
69+
} while (!queue.isEmpty() || emitPromiseRejectionWarnings());
18070
tickInfo[kHasPromiseRejections] = 0;
18171
}
18272

@@ -222,6 +112,8 @@ function setupNextTick() {
222112
args[i - 1] = arguments[i];
223113
}
224114

225-
push(new TickObject(callback, args, getDefaultTriggerAsyncId()));
115+
if (queue.isEmpty())
116+
tickInfo[kHasScheduled] = 1;
117+
queue.push(new TickObject(callback, args, getDefaultTriggerAsyncId()));
226118
}
227119
}

‎node.gyp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@
101101
'lib/internal/constants.js',
102102
'lib/internal/encoding.js',
103103
'lib/internal/errors.js',
104+
'lib/internal/fixed_queue.js',
104105
'lib/internal/freelist.js',
105106
'lib/internal/fs.js',
106107
'lib/internal/http.js',

‎test/parallel/test-fixed-queue.js‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
// Flags: --expose-internals
2+
'use strict';
3+
4+
require('../common');
5+
6+
const assert = require('assert');
7+
const FixedQueue = require('internal/fixed_queue');
8+
9+
{
10+
const queue = new FixedQueue();
11+
assert.strictEqual(queue.head, queue.tail);
12+
assert(queue.isEmpty());
13+
queue.push('a');
14+
assert(!queue.isEmpty());
15+
assert.strictEqual(queue.shift(), 'a');
16+
assert.strictEqual(queue.shift(), null);
17+
}
18+
19+
{
20+
const queue = new FixedQueue();
21+
for (let i = 0; i < 2047; i++)
22+
queue.push('a');
23+
assert(queue.head.isFull());
24+
queue.push('a');
25+
assert(!queue.head.isFull());
26+
27+
assert.notStrictEqual(queue.head, queue.tail);
28+
for (let i = 0; i < 2047; i++)
29+
assert.strictEqual(queue.shift(), 'a');
30+
assert.strictEqual(queue.head, queue.tail);
31+
assert(!queue.isEmpty());
32+
assert.strictEqual(queue.shift(), 'a');
33+
assert(queue.isEmpty());
34+
}

‎test/parallel/test-next-tick-fixed-queue-regression.js‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
5+
// This tests a highly specific regression tied to the FixedQueue size, which
6+
// was introduced in Node.js 9.7.0: https://github.com/nodejs/node/pull/18617
7+
// More specifically, a nextTick list could potentially end up not fully
8+
// clearing in one run through if exactly 2048 ticks were added after
9+
// microtasks were executed within the nextTick loop.
10+
11+
process.nextTick(() => {
12+
Promise.resolve(1).then(() => {
13+
for (let i = 0; i < 2047; i++)
14+
process.nextTick(common.mustCall());
15+
const immediate = setImmediate(common.mustNotCall());
16+
process.nextTick(common.mustCall(() => clearImmediate(immediate)));
17+
});
18+
});

0 commit comments

Comments
 (0)