Skip to content

process: add signal events support#372

Merged
yorkie merged 4 commits intomasterfrom
add/signal
Oct 16, 2018
Merged

process: add signal events support#372
yorkie merged 4 commits intomasterfrom
add/signal

Conversation

@yorkie
Copy link
Copy Markdown
Member

@yorkie yorkie commented Oct 15, 2018

Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added

Added the signal events support by Node.js API, and this is mainly the precondition of simplifying the progress of taking the snapshot, and other debugger/dump usages.

/cc @lolBig @legendecas @fighterkin @algebrait

@legendecas
Copy link
Copy Markdown
Contributor

legendecas commented Oct 16, 2018

Does invocation of process.exit in signal listeners work immediately if a timer or any uv request still waiting?

@yorkie
Copy link
Copy Markdown
Member Author

yorkie commented Oct 16, 2018

The Node.js documentation about process.exit:

Calling process.exit() will force the process to exit as quickly as possible even if there are still asynchronous operations pending that have not yet completed fully, including I/O operations to process.stdout and process.stderr.

So the answer should be yes, process.exit() exits the current process immediately.

@yorkie
Copy link
Copy Markdown
Member Author

yorkie commented Oct 16, 2018

@legendecas Added an issue #373 about the current wrong at process.exit(), and we will address that in another patch.

var signals = require('constants').os.signals;

function testSignal(type) {
process.once(type, function(signal) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

may use common.mustCall to ensure invocation of the callback

@yorkie yorkie force-pushed the add/signal branch 2 times, most recently from 68fa133 to c300f5d Compare October 16, 2018 09:18
Copy link
Copy Markdown
Contributor

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

LGTM

@qile222
Copy link
Copy Markdown
Contributor

qile222 commented Oct 16, 2018

LGTM

@yorkie yorkie merged commit 84bf7f9 into master Oct 16, 2018
@yorkie yorkie deleted the add/signal branch October 16, 2018 12:36
yorkie added a commit that referenced this pull request Oct 16, 2018
qile222 pushed a commit that referenced this pull request Oct 27, 2018
* master:
  build: use CMAKE_INSTALL_PREFIX instead of custom install command (#391)
  child_process: introduce binary protocol to optimizing performance (#393)
  iotjs: cache parsed dump table for error.stack at runtime (#392)
  jerry: merge a59cf4f from upstream to optimize the function call (#381)
  promise: optimize the performance on .then and static resolve (#385)
  jerry: set prototype/constructor for external functions (#386)
  process: add shebang parser supports (#379)
  napi: fix complete callback segv/ticks on async-work (#380)
  jerry: pass regression-test-issue-2105 for heapdump (#382)
  tls, mqtts: fix connection not be actively disconnected (#375)
  websocket: fix the crash due to http exception (#376)
  process: add signal events support (#372)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants