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

Restore SIGPIPE default handling for port children #4905

Draft
wants to merge 1 commit into
base: maint
Choose a base branch
from

Conversation

Kuroneer
Copy link
Contributor

While answering this stackoverflow question I noticed that the default handling for SIGPIPE in open_port children was set to SIG_IGN.

After a little investigation, I found that it's set as such in sys_drivers.c:erl_sys_late_init, and inherited through the fork()s.

This is a problem because if the port owner dies, the pipes attaching the port to the children (the external command) break and the children may not notice it.

The problem arises when the spawned command has no knowledge about this behaviour and ignores the EPIPE when writing or reading from the port's FDs, as the spawned command may rely on the SIGPIPE to terminate and ends orphanded instead.

In this commit, the behaviour is reset to the default just before exec is called in the children. BUT this is a breaking change. Any external command that knowingly (or unknowingly) relied on not being killed when reading from these file descriptors would be killed after this change.

Example

This behaviour can be tested with the following codes:

//dot_sigpipe_dfl
#include <unistd.h>
#include <stdio.h>
#include <signal.h>

int main(int argc, char** argv) {
    if (signal(SIGPIPE, SIG_DFL) != SIG_ERR)
        while(1) {
            fprintf(stdout, ".");
            fflush(stdout);
            sleep(1);
        }
}
//dot
#include <unistd.h>
#include <stdio.h>
#include <signal.h>

int main(int argc, char** argv) {
        while(1) {
            fprintf(stdout, ".");
            fflush(stdout);
            sleep(1);
        }
}
-module(test).
-export([test/0, spawn/1, spawn/2]).

test() ->
    watch0(),
    dot0(),
    signal_dot0(),
    ok.

watch0() ->
    ?MODULE:spawn("watch date", 0).

dot0() ->
    ?MODULE:spawn("./dot", 0).

signal_dot0() ->
    ?MODULE:spawn("./dot_sigpipe_dfl", 0).

spawn(Cmd) ->
    ?MODULE:spawn(Cmd, 0).
spawn(Cmd, WaitTimeMs) ->
    spawn_monitor(fun() ->
                          Port = open_port({spawn, Cmd},[stream, exit_status]),
                          OSPid = proplists:get_value(os_pid, erlang:port_info(Port)),
                          io:format("~p: OS PID for ~s: ~p~n", [self(), Cmd, OSPid]),
                          timer:sleep(WaitTimeMs),
                          io:format("~p: Bye!~n", [self()])
                  end).

Both dot and watch date will get orphanded before this change whereas dot_sigpipe_dfl will terminate correctly regardless.

Alternatives

As an alternative solution, I'd allow providing some sort of signal action option in open_port, but it'd be strange to have an spawned command with signal handling that's not default. If it's decided that this is too breaking, I'd add it to the documentation at least.

During sys_drivers.c:erl_sys_late_init the SIGPIPE handling is set to
ignore. This behaviour shouldn't be inherited for erl_child_setup
children because it differs from the default value, so the handling is
reset to its default just before the exec call.
@Kuroneer
Copy link
Contributor Author

Kuroneer commented May 27, 2021

I have yet to write the test (modifying port_test.c), but I wanted to check before, in case it's better not to fix this

@rickard-green rickard-green added the team:VM Assigned to OTP team VM label May 31, 2021
@garazdawi
Copy link
Contributor

Hello!

I agree with you that it would be better for the child to have the same signal handlers set as is the default of the OS, however (as you also say) this is complicated by backward compatibility since SIGPIPE has been set to IGN since the dawn of time.

There is bound to be some code somewhere that relies on write(1, ...) returning EPIPE and thus knowing that the Erlang emulator has stopped.

I'm not really a fan of adding options to control this behaviour either, but maybe that is the best course action in order to not break people's code. We will have to think about this a bit before making a decision.

@garazdawi garazdawi added the bug Issue is reported as a bug label Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is reported as a bug team:VM Assigned to OTP team VM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants