build: don't squash signal handlers with --shared #10539

sxa · 2016-12-30T13:08:59Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

build

Fixes: #10520
Ref: dd47a8c

An application using node built as a shared library may legitimately
implement it's own signal handling routines. Currenty behaviour is
to squash all signal handlers on node startup. This change will
stop that behaviour when node is built as a shared library.

(Haven't yet checked with the person that raised the problem with me as they're currently on vacation, but this appears to resolve the problem as I understood it from them!)

sam-github · 2016-12-30T15:00:32Z

/to @bnoordhuis

gibfahn · 2017-01-03T16:48:37Z

Nit: it's->its and Currenty->Currently in the commit message

sxa · 2017-01-03T16:59:13Z

Fixed (to current rather than currently)

mhdawson · 2017-01-03T22:17:30Z

src/node.cc

@@ -2192,7 +2192,7 @@ static void WaitForInspectorDisconnect(Environment* env) {
  if (env->inspector_agent()->IsConnected()) {
    // Restore signal dispositions, the app is done and is no longer
    // capable of handling signals.
-#ifdef __POSIX__
+#if defined(__POSIX__) && !defined(NODE_SHARED_MODE)


Can you provide a bit more detail on why its needed here in addition to the other location. The section that avoids restoring them because the parent may have changed them makes sense to me. But this part seems more related to the inspector and may still apply in the shared library case.

I didn't think the inspector applied any additional signal handlers therefore it seems wrong to squash them all on the exit of the node runtime when loaded as a shared library (potentially someone could shutdown the node runtime within their own application and I wouldn't want this to squash any signal handlers in the caller).

I'm happy to be corrected though if there was a separate reason for this loop to be added in here. @eugeneo added this sequence in 6626919 so may have more info and be able to comment.

To be fair, as far as I can see this is only invoked when someone calls process.exit() from within javascript, in which case it'll currently be taking the wrapping process down anyway. on that basis I suspect it won't make a lot of practical difference either way, although I'd prefer to be sure that we really didn't want to squash the handlers in this "embedded" case before removing it.

gibfahn · 2017-01-09T15:12:51Z

ping @eugeneo regarding #10539 (comment)

gibfahn · 2017-01-13T16:48:27Z

ping @bnoordhuis

bnoordhuis

LGTM with a style nit.

bnoordhuis · 2017-01-16T06:19:10Z

src/node.cc

@@ -4106,6 +4107,7 @@ inline void PlatformInit() {
    act.sa_handler = (nr == SIGPIPE) ? SIG_IGN : SIG_DFL;
    CHECK_EQ(0, sigaction(nr, &act, nullptr));
  }
+#endif


#endif // !NODE_SHARED_MODE (two spaces before the slashes)

Fixes: nodejs#10520 Ref: nodejs@dd47a8c An application using node built as a shared library may legitimately implement its own signal handling routines. Current behaviour is to squash all signal handlers on node startup. This change will stop that behaviour when node is built as a shared library.

gibfahn · 2017-01-16T11:26:59Z

CI: https://ci.nodejs.org/job/node-test-commit/7267/ (more of a sanity check really).

Looks like Ben and @ofrobots reviewed 6626919, so unless @eugeneo or @ofrobots have any issue with #10539 (comment)
(i.e. not squashing signal handlers on exiting the shared library node), I think this should be good to go. I'll land this later tonight if there are no objections.

EDIT: CI failed due to a full /tmp directory, rerunning:

CI 2: https://ci.nodejs.org/job/node-test-commit/7269/

gibfahn · 2017-01-17T14:12:27Z

Landed in 0f0f3d3

An application using node built as a shared library may legitimately implement its own signal handling routines. Current behaviour is to squash all signal handlers on node startup. This change will stop that behaviour when node is built as a shared library. PR-URL: #10539 Fixes: #10520 Refs: #615 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

An application using node built as a shared library may legitimately implement its own signal handling routines. Current behaviour is to squash all signal handlers on node startup. This change will stop that behaviour when node is built as a shared library. PR-URL: nodejs#10539 Fixes: nodejs#10520 Refs: nodejs#615 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

An application using node built as a shared library may legitimately implement its own signal handling routines. Current behaviour is to squash all signal handlers on node startup. This change will stop that behaviour when node is built as a shared library. PR-URL: #10539 Fixes: #10520 Refs: #615 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lts-watch-v6.x labels Dec 30, 2016

sam-github approved these changes Dec 30, 2016

View reviewed changes

sxa force-pushed the sharedlib-signalsquash branch from 42cc98f to 669fb0d Compare January 3, 2017 16:58

nodejs-github-bot added the lts-watch-v6.x label Jan 3, 2017

gibfahn approved these changes Jan 3, 2017

View reviewed changes

jasnell approved these changes Jan 3, 2017

View reviewed changes

mhdawson reviewed Jan 3, 2017

View reviewed changes

bnoordhuis approved these changes Jan 16, 2017

View reviewed changes

sxa force-pushed the sharedlib-signalsquash branch from 669fb0d to 4709683 Compare January 16, 2017 11:18

nodejs-github-bot added the lts-watch-v6.x label Jan 16, 2017

gibfahn added the lts-watch-v4.x label Jan 16, 2017

gibfahn self-assigned this Jan 16, 2017

gibfahn approved these changes Jan 16, 2017

View reviewed changes

gibfahn closed this Jan 17, 2017

italoacasas mentioned this pull request Jan 25, 2017

Proposal to update v7.x-staging #10989

Closed

italoacasas mentioned this pull request Jan 29, 2017

v7.5.0 proposal #11062

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Mar 8, 2017

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

MylesBorins mentioned this pull request Mar 9, 2017

v4.8.1 proposal #11760

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: don't squash signal handlers with --shared #10539

build: don't squash signal handlers with --shared #10539

sxa commented Dec 30, 2016 •

edited

Loading

sam-github commented Dec 30, 2016

gibfahn commented Jan 3, 2017

sxa commented Jan 3, 2017

mhdawson Jan 3, 2017

sxa Jan 4, 2017

gibfahn commented Jan 9, 2017

gibfahn commented Jan 13, 2017

bnoordhuis left a comment

bnoordhuis Jan 16, 2017

sxa Jan 16, 2017

gibfahn commented Jan 16, 2017 •

edited

Loading

gibfahn commented Jan 17, 2017

build: don't squash signal handlers with --shared #10539

build: don't squash signal handlers with --shared #10539

Conversation

sxa commented Dec 30, 2016 • edited Loading

Checklist

Affected core subsystem(s)

sam-github commented Dec 30, 2016

gibfahn commented Jan 3, 2017

sxa commented Jan 3, 2017

mhdawson Jan 3, 2017

Choose a reason for hiding this comment

sxa Jan 4, 2017

Choose a reason for hiding this comment

gibfahn commented Jan 9, 2017

gibfahn commented Jan 13, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Jan 16, 2017

Choose a reason for hiding this comment

sxa Jan 16, 2017

Choose a reason for hiding this comment

gibfahn commented Jan 16, 2017 • edited Loading

gibfahn commented Jan 17, 2017

sxa commented Dec 30, 2016 •

edited

Loading

gibfahn commented Jan 16, 2017 •

edited

Loading