-
Notifications
You must be signed in to change notification settings - Fork 29.1k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Adds a flag that helps with debugging deadlocks due to incorrectly implemented `Atomics.wait()` calls. PR-URL: #33292 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
- Loading branch information
Showing
7 changed files
with
178 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
'use strict'; | ||
/* global SharedArrayBuffer */ | ||
|
||
const common = require('../common.js'); | ||
const bench = common.createBenchmark(main, { | ||
n: [1e7] | ||
}); | ||
|
||
function main({ n }) { | ||
const i32arr = new Int32Array(new SharedArrayBuffer(4)); | ||
bench.start(); | ||
for (let i = 0; i < n; i++) | ||
Atomics.wait(i32arr, 0, 1); // Will return immediately. | ||
bench.end(n); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
'use strict'; | ||
require('../common'); | ||
const assert = require('assert'); | ||
const child_process = require('child_process'); | ||
const { Worker } = require('worker_threads'); | ||
|
||
if (process.argv[2] === 'child') { | ||
const i32arr = new Int32Array(new SharedArrayBuffer(8)); | ||
assert.strictEqual(Atomics.wait(i32arr, 0, 1), 'not-equal'); | ||
assert.strictEqual(Atomics.wait(i32arr, 0, 0, 10), 'timed-out'); | ||
|
||
new Worker(` | ||
const i32arr = require('worker_threads').workerData; | ||
Atomics.store(i32arr, 1, -1); | ||
Atomics.notify(i32arr, 1); | ||
Atomics.wait(i32arr, 1, -1); | ||
`, { eval: true, workerData: i32arr }); | ||
|
||
Atomics.wait(i32arr, 1, 0); | ||
assert.strictEqual(Atomics.load(i32arr, 1), -1); | ||
Atomics.store(i32arr, 1, 0); | ||
Atomics.notify(i32arr, 1); | ||
return; | ||
} | ||
|
||
const proc = child_process.spawnSync( | ||
process.execPath, | ||
[ '--trace-atomics-wait', __filename, 'child' ], | ||
{ encoding: 'utf8', stdio: [ 'inherit', 'inherit', 'pipe' ] }); | ||
|
||
if (proc.status !== 0) console.log(proc); | ||
assert.strictEqual(proc.status, 0); | ||
|
||
const SABAddress = proc.stderr.match(/Atomics\.wait\((?<SAB>.+) \+/).groups.SAB; | ||
const actualTimeline = proc.stderr | ||
.replace(new RegExp(SABAddress, 'g'), '<address>') | ||
.replace(new RegExp(`\\(node:${proc.pid}\\) `, 'g'), '') | ||
.replace(/\binf(inity)?\b/gi, 'inf') | ||
.replace(/\r/g, '') | ||
.trim(); | ||
console.log('+++ normalized stdout +++'); | ||
console.log(actualTimeline); | ||
console.log('--- normalized stdout ---'); | ||
|
||
const begin = | ||
`[Thread 0] Atomics.wait(<address> + 0, 1, inf) started | ||
[Thread 0] Atomics.wait(<address> + 0, 1, inf) did not wait because the \ | ||
values mismatched | ||
[Thread 0] Atomics.wait(<address> + 0, 0, 10) started | ||
[Thread 0] Atomics.wait(<address> + 0, 0, 10) timed out`; | ||
|
||
const expectedTimelines = [ | ||
`${begin} | ||
[Thread 0] Atomics.wait(<address> + 4, 0, inf) started | ||
[Thread 1] Atomics.wait(<address> + 4, -1, inf) started | ||
[Thread 0] Atomics.wait(<address> + 4, 0, inf) was woken up by another thread | ||
[Thread 1] Atomics.wait(<address> + 4, -1, inf) was woken up by another thread`, | ||
`${begin} | ||
[Thread 0] Atomics.wait(<address> + 4, 0, inf) started | ||
[Thread 0] Atomics.wait(<address> + 4, 0, inf) was woken up by another thread | ||
[Thread 1] Atomics.wait(<address> + 4, -1, inf) started | ||
[Thread 1] Atomics.wait(<address> + 4, -1, inf) did not wait because the \ | ||
values mismatched`, | ||
`${begin} | ||
[Thread 0] Atomics.wait(<address> + 4, 0, inf) started | ||
[Thread 1] Atomics.wait(<address> + 4, -1, inf) started | ||
[Thread 0] Atomics.wait(<address> + 4, 0, inf) was woken up by another thread | ||
[Thread 1] Atomics.wait(<address> + 4, -1, inf) did not wait because the \ | ||
values mismatched`, | ||
`${begin} | ||
[Thread 0] Atomics.wait(<address> + 4, 0, inf) started | ||
[Thread 0] Atomics.wait(<address> + 4, 0, inf) did not wait because the \ | ||
values mismatched | ||
[Thread 1] Atomics.wait(<address> + 4, -1, inf) started | ||
[Thread 1] Atomics.wait(<address> + 4, -1, inf) did not wait because the \ | ||
values mismatched` | ||
]; | ||
|
||
assert(expectedTimelines.includes(actualTimeline)); |
c7eeef5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible this commit is causing this problem that's showing up in CI?:
c7eeef5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Trott Yes, maybe? It’s a false positive, though – can you check whether adding
default: UNREACHABLE();
to theswitch
statement helps?This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
Sorry, something went wrong.
c7eeef5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current master:
https://ci.nodejs.org/job/node-test-commit-linux-containered/20250/
Adding
default: UNREACHABLE();
: https://ci.nodejs.org/job/node-test-commit-linux-containered/20251/c7eeef5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems to have fixed it. Is that a legitimate fix/workaround that should be PR'ed?
c7eeef5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Trott Hm yeah … it might be better to
return;
instead ofUNREACHABLE();
, or to actually add an “unknown event” message, to account for the fact that V8 might add new event types to that list. Currently the list is exhaustive, but the compiler doesn’t know that, and it’s kinda right to complain about it.c7eeef5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
assert(0)
?c7eeef5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#33451 :)