vm,repl: (add ability to) break on sigint/ctrl+c by addaleax · Pull Request #6635 · nodejs/node · GitHub

Checklist

tests and code linting passes
a test and/or benchmark is included
documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

vm, repl

Description of change

vm:

Adds the breakEvalOnSigint option to vm.runIn(This)Context. This uses a watchdog thread to wait for SIGINT and generally works just like the existing timeout option.
Adds a method to the existing timer-based watchdog to check if it stopped regularly or by running into the timeout. This is used to tell a SIGINT abort from a timer-based one.
Adds (internal) process._{start,stop}SigintWatchdog methods to start/stop the watchdog thread used by the above option manually. This will be used in the REPL to set up SIGINT handling before entering terminal raw mode, so that there is no time window in which Ctrl+C fully aborts the process.

readline:

Return the previous raw mode setting from the internal _setRawMode so that is easier to reset it to its original state later.

repl: Use all that stuff to fix #6612. This probably does not work on Windows as-is.

Current CI: https://ci.nodejs.org/job/node-test-commit/3222/

I think the string here should not reference something platform-specific (pthreads is a POSIX thing).

Current CI is looking good for everything except Windows, but even there at least it compiles.

Not strictly necessary, value->BooleanValue() == false when value->IsUndefined() == true (and you can shorten return value->Booleanvalue() to return value->IsTrue().)

Updated based on @bnoordhuis’ suggestions. Thanks!

Just curious, shouldn't this clear has_pending_signal_?

The tests are failing on Windows, but I suppose that isn't too surprising:

not ok 176 test-process-sigint-watchdog.js
  ---
  duration_ms: 0.132
  ...
not ok 190 test-repl-sigint-nested-eval.js
# TIMEOUT
# process.kill(+process.env.REPL_TEST_PPID, "SIGUSR2");vm.runInThisContext("while(true){}", { breakOnSigint: true });
# > Error: Unknown signal: SIGUSR2
#     at process.kill (internal/process.js:167:15)
#     at repl:1:9
#     at REPLServer.defaultEval (repl.js:295:29)
#     at bound (domain.js:280:14)
#     at REPLServer.runBound [as eval] (domain.js:293:12)
#     at REPLServer.<anonymous> (repl.js:481:10)
#     at emitOne (events.js:96:13)
#     at REPLServer.emit (events.js:188:7)
#     at REPLServer.Interface._onLine (readline.js:222:10)
#     at REPLServer.<anonymous> (readline.js:355:12)
# > 
  ---
  duration_ms: 60.173
  ...
not ok 255 test-vm-sigint.js
# 
# assert.js:90
#   throw new assert.AssertionError({
#   ^
# AssertionError: 'Unknown signal: SIGUSR2' === 'Script execution interrupted.'
#     at ChildProcess.<anonymous> (C:\workspace\node-test-binary-windows\RUN_SUBSET\1\VS_VERSION\vcbt2015\label\win10\test\parallel\test-vm-sigint.js:42:10)
#     at emitTwo (events.js:106:13)
#     at ChildProcess.emit (events.js:191:7)
#     at maybeClose (internal/child_process.js:850:16)
#     at Process.ChildProcess._handle.onexit (internal/child_process.js:215:5)
  ---
  duration_ms: 0.265
  ...
not ok 190 test-repl-sigint.js
# TIMEOUT
# a = 1001;process.kill(+process.env.REPL_TEST_PPID, "SIGUSR2");while(true){}
# > Error: Unknown signal: SIGUSR2
#     at process.kill (internal/process.js:167:15)
#     at repl:1:18
#     at REPLServer.defaultEval (repl.js:295:29)
#     at bound (domain.js:280:14)
#     at REPLServer.runBound [as eval] (domain.js:293:12)
#     at REPLServer.<anonymous> (repl.js:481:10)
#     at emitOne (events.js:96:13)
#     at REPLServer.emit (events.js:188:7)
#     at REPLServer.Interface._onLine (readline.js:222:10)
#     at REPLServer.<anonymous> (readline.js:355:12)
# > 
  ---
  duration_ms: 60.139
  ...
not ok 256 test-vm-timeout.js
# vm.js:18
#   return this.runInContext(context, options);
#               ^
# Error: Script execution interrupted.
#     at Error (native)
#     at ContextifyScript.Script.runInNewContext (vm.js:18:15)
#     at Object.exports.runInNewContext (vm.js:49:17)
#     at context.runInVM (C:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vcbt2015\label\win10\test\parallel\test-vm-timeout.js:29:10)
#     at evalmachine.<anonymous>:1:1
#     at ContextifyScript.Script.runInNewContext (vm.js:18:15)
#     at Object.exports.runInNewContext (vm.js:49:17)
#     at C:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vcbt2015\label\win10\test\parallel\test-vm-timeout.js:32:6
#     at _tryBlock (assert.js:318:5)
#     at _throws (assert.js:337:12)
  ---
  duration_ms: 0.416

The centos5-64 failure looks like a flake, or at least unrelated to this PR.

Updated again.

@bnoordhuis Yeah, I don’t have access to a Windows machine for developing right now. If someone comes along in the next days to check that this works on Windows and/or writes tests for that platform (or I find the opportunity myself), great, but otherwise I’ll disable this feature on Windows for now.

And yep, the centos5-64 failure is a known flaky test.

LGTM (apart from the Windows issues) with an outstanding question on whether or not to clear has_pending_signal_.

@bnoordhuis Oh, yeah, sorry, forgot to answer that one. In the last update, I included that, but a few lines below your comments so it doesn’t need to be duplicated.

Return the previous raw mode setting from the internal `_setRawMode` so that is easier to reset it to its original state later. PR-URL: nodejs#6635 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Adds the ability to stop execution of the current REPL command when receiving SIGINT. This applies only to the default eval function. Fixes: nodejs#6612 PR-URL: nodejs#6635 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

- Adds the `breakEvalOnSigint` option to `vm.runIn(This)Context`. This uses a watchdog thread to wait for SIGINT and generally works just like the existing `timeout` option. - Adds a method to the existing timer-based watchdog to check if it stopped regularly or by running into the timeout. This is used to tell a SIGINT abort from a timer-based one. - Adds (internal) `process._{start,stop}SigintWatchdog` methods to start/stop the watchdog thread used by the above option manually. This will be used in the REPL to set up SIGINT handling before entering terminal raw mode, so that there is no time window in which Ctrl+C fully aborts the process. PR-URL: #6635 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Return the previous raw mode setting from the internal `_setRawMode` so that is easier to reset it to its original state later. PR-URL: #6635 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Adds the ability to stop execution of the current REPL command when receiving SIGINT. This applies only to the default eval function. Fixes: #6612 PR-URL: #6635 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

- Adds the `breakEvalOnSigint` option to `vm.runIn(This)Context`. This uses a watchdog thread to wait for SIGINT and generally works just like the existing `timeout` option. - Adds a method to the existing timer-based watchdog to check if it stopped regularly or by running into the timeout. This is used to tell a SIGINT abort from a timer-based one. - Adds (internal) `process._{start,stop}SigintWatchdog` methods to start/stop the watchdog thread used by the above option manually. This will be used in the REPL to set up SIGINT handling before entering terminal raw mode, so that there is no time window in which Ctrl+C fully aborts the process. PR-URL: #6635 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Return the previous raw mode setting from the internal `_setRawMode` so that is easier to reset it to its original state later. PR-URL: #6635 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Adds the ability to stop execution of the current REPL command when receiving SIGINT. This applies only to the default eval function. Fixes: #6612 PR-URL: #6635 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Notable changes: * buffer: Added `buffer.swap64()` to compliment `swap16()` & `swap32()`. (Zach Bjornson) #7157 * build: New `configure` options have been added for building Node.js as a shared library. (Stefan Budeanu) #6994 - The options are: `--shared`, `--without-v8-platform` & `--without-bundled-v8`. * crypto: Root certificates have been updated. (Ben Noordhuis) #7363 * debugger: The server address is now configurable via `--debug=<address>:<port>`. (Ben Noordhuis) #3316 * npm: Upgraded npm to v3.10.3 (Kat Marchán) #7515 & (Rebecca Turner) #7410 * readline: Added the `prompt` option to the readline constructor. (Evan Lucas) #7125 * repl / vm: `sigint`/`ctrl+c` will now break out of infinite loops without stopping the Node.js instance. (Anna Henningsen) #6635 * src: - Added a `node::FreeEnvironment` public C++ API. (Cheng Zhao) #3098 - Refactored `require('constants')`, constants are now available directly from their respective modules. (James M Snell) #6534 * stream: Improved `readable.read()` performance by up to 70%. (Brian White) #7077 * timers: `setImmediate()` is now up to 150% faster in some situations. (Andras) #6436 * util: Added a `breakLength` option to `util.inspect()` to control how objects are formatted across lines. (cjihrig) #7499 * v8-inspector: Experimental support has been added for debugging Node.js over the inspector protocol. (Ali Ijaz Sheikh) #6792 - *Note: This feature is experimental, and it could be altered or removed.* - You can try this feature by running Node.js with the `--inspect` flag. Refs: #7441 PR-URL: #7550

### Notable changes * **buffer**: Added `buffer.swap64()` to compliment `swap16()` & `swap32()`. (Zach Bjornson) [#7157](nodejs/node#7157) * **build**: New `configure` options have been added for building Node.js as a shared library. (Stefan Budeanu) [#6994](nodejs/node#6994) - The options are: `--shared`, `--without-v8-platform` & `--without-bundled-v8`. * **crypto**: Root certificates have been updated. (Ben Noordhuis) [#7363](nodejs/node#7363) * **debugger**: The server address is now configurable via `--debug=<address>:<port>`. (Ben Noordhuis) [#3316](nodejs/node#3316) * **npm**: Upgraded npm to v3.10.3 (Kat Marchán) [#7515](nodejs/node#7515) & (Rebecca Turner) [#7410](nodejs/node#7410) * **readline**: Added the `prompt` option to the readline constructor. (Evan Lucas) [#7125](nodejs/node#7125) * **repl / vm**: `sigint`/`ctrl+c` will now break out of infinite loops without stopping the Node.js instance. (Anna Henningsen) [#6635](nodejs/node#6635) * **src**: - Added a `node::FreeEnvironment` public C++ API. (Cheng Zhao) [#3098](nodejs/node#3098) - Refactored `require('constants')`, constants are now available directly from their respective modules. (James M Snell) [#6534](nodejs/node#6534) * **stream**: Improved `readable.read()` performance by up to 70%. (Brian White) [#7077](nodejs/node#7077) * **timers**: `setImmediate()` is now up to 150% faster in some situations. (Andras) [#6436](nodejs/node#6436) * **util**: Added a `breakLength` option to `util.inspect()` to control how objects are formatted across lines. (cjihrig) [#7499](nodejs/node#7499) * **v8-inspector**: Experimental support has been added for debugging Node.js over the inspector protocol. (Ali Ijaz Sheikh) [#6792](nodejs/node#6792) - **Note: This feature is _experimental_, and it could be altered or removed.** - You can try this feature by running Node.js with the `--inspect` flag.

addaleax added wip Issues and PRs that are still a work in progress. repl Issues and PRs related to the REPL subsystem. vm Issues and PRs related to the vm subsystem. labels May 8, 2016

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 8, 2016

mscdex reviewed May 8, 2016
View reviewed changes

addaleax force-pushed the repl-break-on-sigint branch 3 times, most recently from 3f8f0bf to 54c035b Compare May 8, 2016 06:58

bnoordhuis reviewed May 8, 2016
View reviewed changes

addaleax force-pushed the repl-break-on-sigint branch from 54c035b to ac45fcc Compare May 8, 2016 11:47

bnoordhuis reviewed May 8, 2016
View reviewed changes

addaleax force-pushed the repl-break-on-sigint branch from ac45fcc to 0cc9fe1 Compare May 9, 2016 06:04

addaleax force-pushed the repl-break-on-sigint branch from 0cc9fe1 to 0484f7f Compare May 12, 2016 16:29

addaleax removed the wip Issues and PRs that are still a work in progress. label May 12, 2016

addaleax added 2 commits June 18, 2016 20:44

readline: return old status from _setRawMode

dc57b9e

Return the previous raw mode setting from the internal `_setRawMode` so that is easier to reset it to its original state later. PR-URL: nodejs#6635 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

repl: break on sigint/ctrl+c

6a93ab1

Adds the ability to stop execution of the current REPL command when receiving SIGINT. This applies only to the default eval function. Fixes: nodejs#6612 PR-URL: nodejs#6635 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

addaleax force-pushed the repl-break-on-sigint branch from 6e44889 to 6a93ab1 Compare June 18, 2016 18:46

addaleax merged commit 6a93ab1 into nodejs:master Jun 18, 2016

addaleax deleted the repl-break-on-sigint branch June 18, 2016 18:51

addaleax added a commit to addaleax/node that referenced this pull request Jun 18, 2016

C++-style mutexes for the freshly landed nodejs#6635

d16e607

This was referenced Jun 18, 2016

src: use RAII for mutexes and condition variables #7334

Merged

Investigate flaky test-vm-timeout.js #6727

Closed

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 18, 2016

addaleax mentioned this pull request Jun 22, 2016

vm: fix nested timeouts with inverse order + fix flaky test-vm-timeout #7373

Closed

3 tasks

Fishrock123 mentioned this pull request Jun 27, 2016

Propose v6.3.0 #7441

Merged

Fishrock123 mentioned this pull request Jul 5, 2016

Propose v6.3.0 (v2) #7550

Merged

cjean-fr mentioned this pull request Jul 7, 2016

Update nodejs to 6.3.0 ScoopInstaller/Scoop#948

Merged

addaleax added the dont-land-on-v4.x label Jul 12, 2016

vm,repl: (add ability to) break on sigint/ctrl+c by addaleax · Pull Request #6635 · nodejs/node · GitHub

Uh oh!

Uh oh!

vm,repl: (add ability to) break on sigint/ctrl+c - #6635

vm,repl: (add ability to) break on sigint/ctrl+c#6635
addaleax merged 3 commits into
nodejs:masterfrom
addaleax:repl-break-on-sigint

addaleax commented May 8, 2016 •
edited
Loading

Uh oh!

Uh oh!

mscdex May 8, 2016

Uh oh!

addaleax commented May 8, 2016

Uh oh!

bnoordhuis May 8, 2016

Uh oh!

addaleax commented May 8, 2016

Uh oh!

bnoordhuis May 8, 2016

Uh oh!

bnoordhuis commented May 8, 2016 •
edited
Loading

Uh oh!

Uh oh!

addaleax commented May 9, 2016

Uh oh!

bnoordhuis commented May 9, 2016

Uh oh!

addaleax commented May 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vm,repl: (add ability to) break on sigint/ctrl+c by addaleax · Pull Request #6635 · nodejs/node · GitHub

Uh oh!

Uh oh!

vm,repl: (add ability to) break on sigint/ctrl+c - #6635

vm,repl: (add ability to) break on sigint/ctrl+c#6635addaleax merged 3 commits intonodejs:masternodejs/node:masterfrom addaleax:repl-break-on-sigintaddaleax/node:repl-break-on-sigintCopy head branch name to clipboard

Conversation

addaleax commented May 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

mscdex May 8, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax commented May 8, 2016

Uh oh!

bnoordhuis May 8, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax commented May 8, 2016

Uh oh!

bnoordhuis May 8, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented May 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented May 9, 2016

Uh oh!

bnoordhuis commented May 9, 2016

Uh oh!

addaleax commented May 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vm,repl: (add ability to) break on sigint/ctrl+c#6635
addaleax merged 3 commits into
nodejs:masterfrom
addaleax:repl-break-on-sigint

addaleax commented May 8, 2016 •
edited
Loading

bnoordhuis commented May 8, 2016 •
edited
Loading