src: fix race condition in debug signal on exit by bnoordhuis · Pull Request #3528 · nodejs/node · GitHub

Before this commit, sending a SIGUSR1 at program exit could trigger a
hard to reproduce race condition where v8::Debug::DebugBreak(isolate)
got called when the isolate was in the process of being torn down.

A similar race condition is in theory possible when sending signals
to two threads simultaneously but I haven't been able to reproduce
that myself (and I tried, oh how I tried.)

This commit fixes the race condition by turning node_isolate into
a std::atomic and using it as an ad hoc synchronization primitive
in places where that is necessary.

R=@indutny?

CI: https://ci.nodejs.org/job/node-test-pull-request/615/

Hmmm, our linter doesn't complain about this?

Why would it? It's a common C++ idiom.

LGTM

OS X... ../src/node.cc:89:10: fatal error: 'atomic' file not found. bangs head against table

@bnoordhuis .. ugh.

Hacked together a crummy workaround. I'll refine it further but first let's see if the basic approach works: https://ci.nodejs.org/job/node-test-pull-request/619/

Alternate take: https://ci.nodejs.org/job/node-test-pull-request/621/

Looking healthy. @indutny @jasnell Can I get a LGTM for 1b0ffcc?

Having to polyfill atomic makes me sad. Otherwise LGTM.

Nice polyfill you have here.

LGTM, if it works

Before this commit, sending a SIGUSR1 at program exit could trigger a hard to reproduce race condition where `v8::Debug::DebugBreak(isolate)` got called when the isolate was in the process of being torn down. A similar race condition is in theory possible when sending signals to two threads simultaneously but I haven't been able to reproduce that myself (and I tried, oh how I tried.) This commit fixes the race condition by turning `node_isolate` into a `std::atomic` and using it as an ad hoc synchronization primitive in places where that is necessary. A bare minimum std::atomic polyfill is added for OS X because Apple wouldn't be Apple if things just worked out of the box. PR-URL: nodejs#3528 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: James M Snell <jasnell@gmail.com>

A bare minimum std::atomic polyfill is added for OS X because Apple
wouldn't be Apple if things just worked out of the box.

lol

@bnoordhuis ... is this something that should go into LTS or not?

Before this commit, sending a SIGUSR1 at program exit could trigger a hard to reproduce race condition where `v8::Debug::DebugBreak(isolate)` got called when the isolate was in the process of being torn down. A similar race condition is in theory possible when sending signals to two threads simultaneously but I haven't been able to reproduce that myself (and I tried, oh how I tried.) This commit fixes the race condition by turning `node_isolate` into a `std::atomic` and using it as an ad hoc synchronization primitive in places where that is necessary. A bare minimum std::atomic polyfill is added for OS X because Apple wouldn't be Apple if things just worked out of the box. PR-URL: #3528 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Landed in v4.x-staging in c85736e

Before this commit, sending a SIGUSR1 at program exit could trigger a hard to reproduce race condition where `v8::Debug::DebugBreak(isolate)` got called when the isolate was in the process of being torn down. A similar race condition is in theory possible when sending signals to two threads simultaneously but I haven't been able to reproduce that myself (and I tried, oh how I tried.) This commit fixes the race condition by turning `node_isolate` into a `std::atomic` and using it as an ad hoc synchronization primitive in places where that is necessary. A bare minimum std::atomic polyfill is added for OS X because Apple wouldn't be Apple if things just worked out of the box. PR-URL: nodejs#3528 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Before this commit, sending a SIGUSR1 at program exit could trigger a hard to reproduce race condition where `v8::Debug::DebugBreak(isolate)` got called when the isolate was in the process of being torn down. A similar race condition is in theory possible when sending signals to two threads simultaneously but I haven't been able to reproduce that myself (and I tried, oh how I tried.) This commit fixes the race condition by turning `node_isolate` into a `std::atomic` and using it as an ad hoc synchronization primitive in places where that is necessary. A bare minimum std::atomic polyfill is added for OS X because Apple wouldn't be Apple if things just worked out of the box. PR-URL: #3528 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: James M Snell <jasnell@gmail.com>

bnoordhuis added c++ Issues and PRs that require attention from people who are familiar with C++. debugger labels Oct 26, 2015

thefourtheye reviewed Oct 26, 2015
View reviewed changes

jasnell added the lts-watch-v4.x label Oct 26, 2015

bnoordhuis force-pushed the fix-debugger-race branch from c25388b to 1b0ffcc Compare October 26, 2015 18:02

indutny reviewed Oct 26, 2015
View reviewed changes

bnoordhuis force-pushed the fix-debugger-race branch from 1b0ffcc to 53e64bb Compare October 27, 2015 12:01

bnoordhuis closed this Oct 27, 2015

bnoordhuis deleted the fix-debugger-race branch October 27, 2015 12:02

bnoordhuis merged commit 53e64bb into nodejs:master Oct 27, 2015

rvagg mentioned this pull request Oct 28, 2015

Preparing v4.2.2 LTS Maintenance nodejs/Release#50

Closed

jasnell added land-on-v4.x and removed lts-watch-v4.x labels Oct 28, 2015

rvagg mentioned this pull request Oct 29, 2015

Release proposal: v5.0.0 #3466

Merged

jasnell mentioned this pull request Oct 29, 2015

Release Proposal: v4.2.2 "Argon" (LTS) #3588

Closed

jasnell removed the land-on-v4.x label Nov 3, 2015

aliscco mentioned this pull request Feb 11, 2024

[Snyk] Security upgrade lerna from 3.22.1 to 6.5.0 aliscco/alisco-node#1233

Open

aliscco mentioned this pull request Mar 23, 2024

[Snyk] Security upgrade lerna from 3.22.1 to 6.5.0 aliscco/alisco-node#1267

Open

aliscco mentioned this pull request May 14, 2024

[Snyk] Security upgrade lerna from 3.22.1 to 6.5.0 aliscco/alisco-node#1437

Open

src: fix race condition in debug signal on exit by bnoordhuis · Pull Request #3528 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: fix race condition in debug signal on exit - #3528

src: fix race condition in debug signal on exit#3528bnoordhuis merged 1 commit intonodejs:masternodejs/node:masterfrom bnoordhuis:fix-debugger-racebnoordhuis/io.js:fix-debugger-raceCopy head branch name to clipboard

Conversation

bnoordhuis commented Oct 26, 2015

Uh oh!

thefourtheye Oct 26, 2015

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Oct 26, 2015

Choose a reason for hiding this comment

Uh oh!

indutny commented Oct 26, 2015

Uh oh!

jasnell commented Oct 26, 2015

Uh oh!

bnoordhuis commented Oct 26, 2015

Uh oh!

jasnell commented Oct 26, 2015

Uh oh!

bnoordhuis commented Oct 26, 2015

Uh oh!

bnoordhuis commented Oct 26, 2015

Uh oh!

bnoordhuis commented Oct 26, 2015

Uh oh!

jasnell commented Oct 26, 2015

Uh oh!

indutny Oct 26, 2015

Choose a reason for hiding this comment

Uh oh!

indutny commented Oct 26, 2015

Uh oh!

thefourtheye commented Oct 27, 2015

Uh oh!

jasnell commented Oct 28, 2015

Uh oh!

jasnell commented Oct 28, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

src: fix race condition in debug signal on exit#3528
bnoordhuis merged 1 commit into
nodejs:masterfrom
bnoordhuis:fix-debugger-race