mpv icon indicating copy to clipboard operation
mpv copied to clipboard

terminal_thread: only act on last command in stop_cont_pipe

Open millert opened this issue 1 year ago • 5 comments

It is possible for stop_cont_sighandler() to be called more than once before terminal_thread can act on the command. For example, if mpv receives multiple SIGTSTP, it will suspend and when resumed will suspend again immediately. Draining the pipe (which is non-blocking) works around this.

On OpenBSD, SIGTSTP appears to be sent to each thread, which makes it impossible to resume mpv since there will always be another PIPE_STOP byte waiting in the pipe.

millert avatar Aug 09 '24 16:08 millert

Make sense. @N-R-K do you have opinion on that?

kasper93 avatar Aug 10 '24 21:08 kasper93

If you act only on the last command in the pipe then it'll miss earlier (but different) signals if they were sent in quick succession (not sure if it'd happen in practice, maybe from some script).

This also seems racy, consider the following ordering:

  1. N threads are active and receive the signal
  2. N-1 threads have written to the pipe
  3. The reader drains the pipe
  4. Nth thread writes to the pipe
  5. Now there's an extra STOP command lingering in the pipe still

On OpenBSD, SIGTSTP appears to be sent to each thread

Is openbsd the only platform that does this? If yes, is there a way to disable this behavior instead?

N-R-K avatar Aug 10 '24 21:08 N-R-K

If you act only on the last command in the pipe then it'll miss earlier (but different) signals if they were sent in quick succession

Nevermind this, the other signals are using a separate pipe (death_pipe).

This also seems racy [...]

Is openbsd the only platform that does this?

These still stand though. And from skimming through this thread and some conversation elsewhere, I'm under the impression that sending signal to each thread rather than each process is the buggy behavior here. posix specifies the latter:

"[...] receipt of the SUSP character shall cause a SIGTSTP signal to be sent to all processes in the foreground process group for which the terminal is the controlling terminal [...]"

N-R-K avatar Aug 11 '24 15:08 N-R-K

Well, a process is just a collection of associated threads. POSIX says "Signals generated for the process shall be delivered to exactly one of those threads within the process which is in a call to a sigwait() function selecting that signal or has not blocked delivery of the signal." From my reading of POSIX the OpenBSD behavior appears to be non-conforming, but there is active work happening in OpenBSD with respect to signal handling and threads so this should be resolved eventually there.

millert avatar Aug 12 '24 15:08 millert