Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jun 18, 2016

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented May 8, 2016

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/

@addaleax 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 nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 8, 2016
int ret = SigintWatchdogHelper::GetInstance()->Start();
if (ret != 0) {
Environment* env = Environment::GetCurrent(args);
env->ThrowErrnoException(ret, "pthread_create");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

addaleax commented May 8, 2016

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

Local<Value> value = args[i].As<Object>()->Get(key);
if (value->IsUndefined()) {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

addaleax commented May 8, 2016

Updated based on @bnoordhuis’ suggestions. Thanks!

uv_mutex_lock(&mutex_);
uv_mutex_lock(&list_mutex_);

bool had_pending_signal = has_pending_signal_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, shouldn't this clear has_pending_signal_?

@bnoordhuis
Copy link
Member

bnoordhuis commented May 8, 2016

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.

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

addaleax commented May 9, 2016

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.

@bnoordhuis
Copy link
Member

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

@addaleax
Copy link
Member Author

addaleax commented May 9, 2016

@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.

@addaleax addaleax force-pushed the repl-break-on-sigint branch from 0cc9fe1 to 0484f7f Compare May 12, 2016 16:29
@addaleax 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
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>
@addaleax addaleax force-pushed the repl-break-on-sigint branch from 6e44889 to 6a93ab1 Compare June 18, 2016 18:46
@addaleax addaleax merged commit 6a93ab1 into nodejs:master Jun 18, 2016
@addaleax 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
@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 18, 2016
Fishrock123 pushed a commit that referenced this pull request Jun 27, 2016
- 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>
Fishrock123 pushed a commit that referenced this pull request Jun 27, 2016
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>
Fishrock123 pushed a commit that referenced this pull request Jun 27, 2016
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>
@Fishrock123 Fishrock123 mentioned this pull request Jun 27, 2016
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
- 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>
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
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>
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
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>
@Fishrock123 Fishrock123 mentioned this pull request Jul 5, 2016
Fishrock123 added a commit that referenced this pull request Jul 6, 2016
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
Fishrock123 added a commit that referenced this pull request Jul 6, 2016
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
Fishrock123 added a commit that referenced this pull request Jul 6, 2016
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
lukesampson pushed a commit to ScoopInstaller/Scoop that referenced this pull request Jul 7, 2016
### 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. repl Issues and PRs related to the REPL subsystem. semver-minor PRs that contain new features and should be released in the next minor version. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CTRL-C does not quit repl when in infinte loop
5 participants