test: shared lib build doesn't handle SIGPIPE #19211

yhwang · 2018-03-07T20:38:48Z

For shared lib build, we leave the signal handling for embedding users.
In these two test cases:

parallel/test-process-external-stdio-close-spawn
parallel/test-process-external-stdio-close

The pipe is used for stdout and is destroied before child process uses
it for logging. So the node executble that uses shared lib build
receives SIGPIPE and the child process ends. Need to modify these two
test cases to check the SIGPIPE instead.

Refs: #18535

Signed-off-by: Yihong Wang [email protected]

Checklist

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

gireeshpunathil · 2018-03-08T04:11:37Z

@yhwang : can you please clarify - which one is embedded here: the parent or the child, or both?

if parent, the child's exit code and signal are of no difference, right?
if child, the proposed assertions on child will form expectations from unknown right?
if both, how is it different from the existing code?

bnoordhuis · 2018-03-08T08:58:20Z

This doesn't look correct to me. The signal should be handled by the binary because otherwise it's practically useless, the first aborted connection would kill it.

yhwang · 2018-03-08T16:29:44Z

The case here is both parent and child node executable are using shared lib. And the related code for signal handling is here: https://github.com/nodejs/node/blob/master/src/node.cc#L4103-L4117 and it's surrounded by #ifndef NODE_SHARED_MODE, which means shared lib users should handle that by their own.

For these two test cases, the parent create pipes for child's stdin, stdout and stderr. But after spawn, parent destroys the stdout. And in the console.js, it catches the error and ignore it here: https://github.com/nodejs/node/blob/master/lib/console.js#L110-L132 . So, in regular build (not using shared lib), when the child tries to write to stdout in line 13: console.log('logging should not cause a crash'); it hits the SIGPIPE and handle it (actually, ignores it) then the child process exits with status code 0 and null for term_signal.

Because the case here is both parent and child are shared lib build, the child doesn't have signal handler registration (code in node.cc L110-L132 that I mentioned above), so the child directly exits when it writes to the stdout. The exit code and term signal that parent receives are status code: 1 and term sign: SIGPIPE.

I think there are 2 ways to fix this, one is to modify the test case like what I did in this change. And another one is to modify the node_main.cc to handle the signal if it's shared lib build (#ifdef NODE_SHARED_MODE). Because I just borrow main() function from node_main.cc for the shared lib build, I don't think changing node_main.cc for this case is a good idea. So, I chose the first one :-).

bnoordhuis · 2018-03-08T17:08:11Z

I understand what you did and why but IMO it's papering over the real issue. This PR fixes the tests but it's only a matter of time before it reappears. I suggest moving some of the signal handling code from node.cc to node_main.cc.

yhwang · 2018-03-08T17:44:47Z

@bnoordhuis I agree with you totally. However, for now, the node_main.cc is used to build the executable for shared lib temporally. We should have an example node_shared_main.cc for the embedding users and also provide some tips about how to build the shared lib, link the shared lib and what things they need to handle when using shared lib.

If you think node_shared_main.cc is not necessary, I can refactor the signal handling code in node.cc as a public API and call it in node_main.cc for shared lib case.

bnoordhuis · 2018-03-08T18:53:55Z

I'd start out simple and move the sigaction(SIGPIPE, ...) to node_main.cc, that fixes the immediate issue.

yhwang · 2018-03-08T19:00:19Z

@bnoordhuis sure! For double check,

move the sigaction(SIGPIPE, ...) to node_main.cc

I guess you were saying copy the sigaction(SIGPIPE,...) to node_main.cc with #ifdef NODE_SHARED_MODE, right?

bnoordhuis · 2018-03-08T19:33:55Z

I guess you were saying copy the sigaction(SIGPIPE,...) to node_main.cc with #ifdef NODE_SHARED_MODE, right?

No, because both builds need it. If you move it, you can drop the corresponding code from node.cc that's currently guarded by a #ifndef NODE_SHARED_MODE.

It's currently a loop that does this:

if (nr == SIGKILL || nr == SIGSTOP) continue;
act.sa_handler = (nr == SIGPIPE) ? SIG_IGN : SIG_DFL;
CHECK_EQ(0, sigaction(nr, &act, nullptr));

And you could change that to:

if (nr == SIGKILL || nr == SIGSTOP || nr == SIGPIPE) continue;
act.sa_handler = SIG_DFL;
CHECK_EQ(0, sigaction(nr, &act, nullptr));

But please leave a comment explaining why SIGPIPE can safely be skipped.

I suppose a case could be made that much of PlatformInit() properly belongs in node_main.cc rather than node.cc but let's start small.

yhwang · 2018-03-08T20:10:35Z

I see. But only move SIGPIPE out of the #ifndef NODE_SHARED_MODE section seems not a good idea. In that case, it separates the PlatformInit() logic into two places and because of two test case hit the SIGPIPE in the shared lib build. Is it worthy?

I think only adding a section in node_main.cc would be more obvious and clear (because it would be surrounded by the #ifdef NODE_SHARED_MODE.

And in the future, if the PlatformInit() could be moved to node_main.cc we won't need to put SIGPIPE back, right?

Another approach is to suppress these 2 test cases for share lib build.

For shared lib build, we leave the signal handling for embedding users. In these two test cases: - `parallel/test-process-external-stdio-close-spawn` - `parallel/test-process-external-stdio-close` The pipe is used for stdout and is destroied before child process uses it for logging. So the node executble that uses shared lib build receives SIGPIPE and the child process ends. This change ignores the SIGPIPE in node_main.cc for shared lib case. Refs: nodejs#18535 Signed-off-by: Yihong Wang <[email protected]>

yhwang · 2018-03-09T01:13:52Z

@bnoordhuis I reworked on the change and added a section in node_main.cc to ignore SIGPIPE and put comment to explain about the change. Hope it's easier and clearer for the node::PlatformInit() move in the future.

PR changed completely

mhdawson

LGTM

yhwang · 2018-03-09T17:37:11Z

Thank you for the review!
CI: https://ci.nodejs.org/job/node-test-commit/16816/

yhwang · 2018-03-09T19:10:28Z

smartos-15-64 failed with these logs:

FATAL: java.io.IOException: Connection reset by peer
java.io.IOException: Connection reset by peer
.........
no workspace for node-test-commit-smartos/nodes=smartos15-64 #15952

But other smartos builds passed. I think the results are good.

gireeshpunathil · 2018-03-10T05:10:30Z

My understanding is that in SHLIB mode, absolutely no signal handling is carried out, and the responsibility is delegated to the embedder. Please confirm this.

yhwang · 2018-03-12T16:47:34Z

@gireeshpunathil

in SHLIB mode, absolutely no signal handling is carried out, and the responsibility is delegated to the embedder

Yes, squashing signal handlers is only applied to non-shared lib build. Here is the commit: 0f0f3d3 .

This PR modifies node_main.cc to handle SIGPIPE and is not related shared lib. (shared lib doesn't include node_main.cc)

For shared lib build, we leave the signal handling for embedding users. In these two test cases: - `parallel/test-process-external-stdio-close-spawn` - `parallel/test-process-external-stdio-close` The pipe is used for stdout and is destroied before child process uses it for logging. So the node executble that uses shared lib build receives SIGPIPE and the child process ends. This change ignores the SIGPIPE in node_main.cc for shared lib case. Refs: #18535 Signed-off-by: Yihong Wang <[email protected]> PR-URL: #19211 Refs: #18535 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

yhwang · 2018-03-13T00:14:27Z

Landed in ffd618b and closed this PR.

For shared lib build, we leave the signal handling for embedding users. In these two test cases: - `parallel/test-process-external-stdio-close-spawn` - `parallel/test-process-external-stdio-close` The pipe is used for stdout and is destroied before child process uses it for logging. So the node executble that uses shared lib build receives SIGPIPE and the child process ends. This change ignores the SIGPIPE in node_main.cc for shared lib case. Refs: #18535 Signed-off-by: Yihong Wang <[email protected]> PR-URL: #19211 Refs: #18535 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

For shared lib build, we leave the signal handling for embedding users. In these two test cases: - `parallel/test-process-external-stdio-close-spawn` - `parallel/test-process-external-stdio-close` The pipe is used for stdout and is destroied before child process uses it for logging. So the node executble that uses shared lib build receives SIGPIPE and the child process ends. This change ignores the SIGPIPE in node_main.cc for shared lib case. Refs: nodejs#18535 Signed-off-by: Yihong Wang <[email protected]> PR-URL: nodejs#19211 Refs: nodejs#18535 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

For shared lib build, we leave the signal handling for embedding users. In these two test cases: - `parallel/test-process-external-stdio-close-spawn` - `parallel/test-process-external-stdio-close` The pipe is used for stdout and is destroied before child process uses it for logging. So the node executble that uses shared lib build receives SIGPIPE and the child process ends. This change ignores the SIGPIPE in node_main.cc for shared lib case. Refs: #18535 Signed-off-by: Yihong Wang <[email protected]> PR-URL: #19211 Refs: #18535 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Signed-off-by: Beth Griggs <[email protected]>

nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 7, 2018

gireeshpunathil added the embedding Issues and PRs related to embedding Node.js in another project. label Mar 8, 2018

jasnell previously approved these changes Mar 8, 2018

View reviewed changes

yhwang force-pushed the sharedlib-test-child-fork-pipe branch from 24c5de4 to c89b462 Compare March 9, 2018 01:05

yhwang force-pushed the sharedlib-test-child-fork-pipe branch from c89b462 to 4a7be07 Compare March 9, 2018 01:10

bnoordhuis approved these changes Mar 9, 2018

View reviewed changes

cjihrig approved these changes Mar 9, 2018

View reviewed changes

jasnell approved these changes Mar 9, 2018

View reviewed changes

mhdawson approved these changes Mar 9, 2018

View reviewed changes

yhwang added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 9, 2018

gireeshpunathil approved these changes Mar 10, 2018

View reviewed changes

yhwang closed this Mar 13, 2018

yhwang deleted the sharedlib-test-child-fork-pipe branch March 13, 2018 00:14

yhwang mentioned this pull request Mar 13, 2018

Some test cases fail when the node executable is built by --shared #18535

Closed

7 tasks

targos mentioned this pull request Mar 18, 2018

v9.9.0 proposal #19428

Merged

MylesBorins mentioned this pull request Mar 26, 2019

v8.16.0 proposal #26933

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: shared lib build doesn't handle SIGPIPE #19211

test: shared lib build doesn't handle SIGPIPE #19211

yhwang commented Mar 7, 2018

gireeshpunathil commented Mar 8, 2018

bnoordhuis commented Mar 8, 2018

yhwang commented Mar 8, 2018 •

edited

Loading

bnoordhuis commented Mar 8, 2018

yhwang commented Mar 8, 2018

bnoordhuis commented Mar 8, 2018

yhwang commented Mar 8, 2018

bnoordhuis commented Mar 8, 2018

yhwang commented Mar 8, 2018 •

edited

Loading

yhwang commented Mar 9, 2018

mhdawson left a comment

yhwang commented Mar 9, 2018

yhwang commented Mar 9, 2018

gireeshpunathil commented Mar 10, 2018

yhwang commented Mar 12, 2018

yhwang commented Mar 13, 2018

test: shared lib build doesn't handle SIGPIPE #19211

test: shared lib build doesn't handle SIGPIPE #19211

Conversation

yhwang commented Mar 7, 2018

Checklist

gireeshpunathil commented Mar 8, 2018

bnoordhuis commented Mar 8, 2018

yhwang commented Mar 8, 2018 • edited Loading

bnoordhuis commented Mar 8, 2018

yhwang commented Mar 8, 2018

bnoordhuis commented Mar 8, 2018

yhwang commented Mar 8, 2018

bnoordhuis commented Mar 8, 2018

yhwang commented Mar 8, 2018 • edited Loading

yhwang commented Mar 9, 2018

mhdawson left a comment

Choose a reason for hiding this comment

yhwang commented Mar 9, 2018

yhwang commented Mar 9, 2018

gireeshpunathil commented Mar 10, 2018

yhwang commented Mar 12, 2018

yhwang commented Mar 13, 2018

yhwang commented Mar 8, 2018 •

edited

Loading

yhwang commented Mar 8, 2018 •

edited

Loading