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

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.

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 :-).

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.

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

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

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

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.

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.

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

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

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.

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.

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

Landed in ffd618b and closed this PR.