janestreet / async_unix

Jane Street Capital's asynchronous execution library (unix)
MIT License
33 stars 21 forks source link

Ignore SIGPIPE by default? #2

Closed avsm closed 10 years ago

avsm commented 10 years ago

Right now SIGPIPE exceptions can leak out and kill the whole server process in Cohttp_async. There's no real reason to not ignore SIGPIPE these days, since the next read/write will result in an error in the return code anyway.

Can it be blocked globally when linking Async?

("unhandled exception"
 ((lib/monitor.ml.Error_
   ((exn (Unix.Unix_error "Broken pipe" writev_assume_fd_is_nonblocking ""))
    (backtrace
     ("Raised by primitive operation at file \"lib/bigstring.ml\", line 275, characters 2-56"
      "Called from file \"lib/result.ml\", line 101, characters 10-14"
      "Re-raised at file \"lib/result.ml\", line 108, characters 23-26"
      "Called from file \"lib/raw_fd.ml\", line 261, characters 11-25"
      "Re-raised at file \"lib/writer0.ml\", line 578, characters 49-52"
      "Called from file \"lib/scheduler.ml\", line 139, characters 6-17"
      "Called from file \"lib/jobs.ml\", line 65, characters 8-13" ""))
    (monitor
     (((name
        (writer
         (3
          ((file_descr 6)
           (info
            (socket
             ((listening_on
               ((type_
                 ((family
                   ((family PF_INET) (address_of_sockaddr_exn <fun>)
                    (sexp_of_address <fun>)))
                  (socket_type SOCK_STREAM)))
                (fd
                 ((file_descr 3)
                  (info
                   (replaced
                    (listening
                     (previously_was
                      (replaced
                       ((socket (bound_on (Inet (0.0.0.0 8080))))
                        (previously_was
                         (socket
                          ((family
                            ((family PF_INET) (address_of_sockaddr_exn <fun>)
                             (sexp_of_address <fun>)))
                           (socket_type SOCK_STREAM))))))))))
                  (kind (Socket Passive)) (supports_nonblock true)
                  (have_set_nonblock true) (state (Close_requested <fun>))
                  (watching ((read Stop_requested) (write Not_watching)))
                  (watching_has_changed true) (num_active_syscalls 1)
                  (close_finished Empty)))))
              (client (Inet (127.0.0.1 50021))))))
           (kind (Socket Active)) (supports_nonblock true)
           (have_set_nonblock true) (state Closed)
           (watching ((read Not_watching) (write Not_watching)))
           (watching_has_changed false) (num_active_syscalls 0)
           (close_finished (Full ()))))))
       (here ()) (id 52) (has_seen_error true) (someone_is_listening true)
       (kill_index 0))
      ((name main) (here ()) (id 1) (has_seen_error true)
       (someone_is_listening false) (kill_index 0))))))
  (Pid 79172)))
ghost commented 10 years ago

Actually it is already the case: Scheduler.go () does manage SIGPIPE. In the documentation:

    [go ()] calls [handle_signal Sys.sigpipe], which causes the SIGPIPE signal to be
    ignored.  Low-level syscalls (e.g. write) still raise EPIPE.

Async installs a signal handler instead of setting the signal disposition to ignore, but the result is the same: write will fail with EPIPE instead of the process being terminated. That's what is shown in the backtrace.

avsm commented 10 years ago

Got it; I just wasn't ignoring handler errors correctly; now fixed and confirmed that all's working. thanks!