crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.32k stars 1.61k forks source link

Child processes should have SIGPIPE reset before spawning. #6527

Open Timbus opened 6 years ago

Timbus commented 6 years ago

Scroll down: https://github.com/crystal-lang/crystal/issues/6527#issuecomment-412855603

There are a few issues in the backlog that touch on this; https://github.com/crystal-lang/crystal/issues/408 and https://github.com/crystal-lang/crystal/issues/2961

Also see my notes here: https://github.com/crystal-lang/crystal/pull/6518#issuecomment-412247289

Test scenario: crystal eval 'sleep 1; puts "hi"' | echo 'prepare to die!' --> :boom:

I think Crystal should be handling SIGPIPE by default and either exiting a little more cleanly, ~or marking the relevant IO handle as closed and continuing~ (edit: maybe don't do that).

Also, manually trapping Signal::PIPE didn't seem to fix this in my testing.


This is similar to, but not the same as https://github.com/crystal-lang/crystal/issues/6359 as a closed FD does not receive a SIGPIPE. In this setup, it appears Crystal is just (somehow) forcing the FD back open when it sets up epoll, which causes the exact same error on a write.

ysbaddaden commented 6 years ago

We never receive SIGPIPE. It's always ignored.

What's happening is a EPIPE errno when trying to write to a closed IO, which is expected behavior.

Timbus commented 6 years ago

I can assume crystal ignores sigpipe for socket IO reasons, based on the other issues. I don't like that. You can make sockets not generate a sigpipe, which should be more ideal.

At the very least, a way to toggle sigpipe back on would be useful for people writing command line programs, although someone writing a crystal variant of 'socat' will be in for a rough time.

ysbaddaden commented 6 years ago

SIGPIPE is ignored by default but can probably be reset/trap if you really need to. I don't see the point of an asynchronous SIGPIPE delivery over raising an EPIPE error when the error does happen.

Failing to read from STDIN or to write to STDOUT/STDERR are errors and that must be reported.

ysbaddaden commented 6 years ago

At the very least to report that they failed because the IO is closed and associated resources can be cleaned up in the exception handling chain.

ysbaddaden commented 6 years ago

I don't understand why socat would be impossible with EPIPE. Quite the opposite.

Timbus commented 6 years ago

Hmmmm. After sleeping on it, I guess so. I just feels surprising to the user (me) that a program doesn't shut down with a sigpipe, when the end pipe is closed. On the other hand, crystal still does shut down with an exception, so maybe this is the actually the best middle ground.

I'd at least appreciate it if it was documented somewhere?

ysbaddaden commented 6 years ago

Well, the point of EPIPE is to not crash a process with SIGPIPE, but report the issue in context, eventually exiting if not rescued. Without that, we can't know that the other end of a file descriptor got closed, and SIGPIPE would exit the process (outch). Maybe it does pass the faulting fd, but we'd have to deal with it asynchronously, which is already a pain with SIGCHLD

Ruby, and I assume other languages, ignore SIGPIPE to get a synchronous EPIPE in context.

Now, if you need the default behavior for special cases, maybe Signal::PIPE.reset does the job? That or rescuing Errno and checking for EPIPE to silence a backtrace.

Timbus commented 6 years ago

I have written programs that relied on sigpipe in the past, as they were designed to be pipeline utilities. I know it's a bit lazy but that's what posix made it for. In the end I can handle catching the EPIPE, I just didn't think I had to.


Your mention of SIGCHLD somehow worried me though.. If Crystal is ignoring SIGPIPE, what about the child processes? That means they are also ignoring the signal?

And a quick test program confirms it: crystal eval 'Process.run("./pcat", ["hugefile"], output: Process::Redirect::Inherit)' | head -n1 .. where pcat is an extremely inefficient cat that I just wrote in perl. Doesn't get killed.

Annnd a quick google uncovers this for python: https://bugs.python.org/issue1652

That's not so good..

ysbaddaden commented 6 years ago

I guess we must reset it after fork before exec, too?

Timbus commented 6 years ago

Yep. Too lazy to make new issue. Will reopen and rename this one