mpv-player / mpv

🎥 Command line video player
https://mpv.io
Other
28.02k stars 2.88k forks source link

run command in input.conf doesn't quit anymore #8377

Open Flurrywinde opened 3 years ago

Flurrywinde commented 3 years ago

Important Information

Provide following Information:

mpv 0.33.0-31-gb8156a9a86

Ubuntu 20.04.1

Compiled locally using mpv-build

It worked before I upgraded from Ubuntu 18.04 to 20.04, which seems to have updated mpv as well.

i3 version 4.19 (2020-11-15)

Reproduction steps

Same problem occurs without using /bin/sh:

My input.conf file used to do it this way:

i run "mpv-info" "${working-directory}" "${path}"

where mpv-info is just a script that calls dzen2 with its -p parameter to automatically exit after a certain period of time. This used to work and a call to mpv-info still works if called from the command line.

Expected behavior

dzen2 -p 1 should self-terminate after one second.

Actual behavior

dzen2 persists longer than one second, persists after mpv is closed, and has to be killed with kill -9

Log file

output.txt

avih commented 3 years ago

It worked before I upgraded from Ubuntu 18.04 to 20.04, which seems to have updated mpv as well.

I can reproduce the issue and for now don't know the cause, but did you consider that upgrading ubuntu also updated dzen2, and that's the actual cause? I don't think the run command in mpv changed.

There is somethins strange though. If i add i run xterm then it does indeed launch xterm, but the close button doesn't close xterm. Typing exit at the shell inside xterm does exit it. However, if i use i run mlterm then mlterm does close with the X button.

I don't know if the issue is limited to x applications, or maybe some environment, but it does appear that running at least x applications using the run command is different in some ways to launching them normally from the shell.

avih commented 3 years ago

I bisected it, and the issue (with dzen2 and xterm not exiting) starts with this commit: f2c7c641b38d060a7eea52a92bc0239f1603bffb .

I tried to remove the setsid() call mentioned at the commit message to keep it attached to the terminal, but that did not seem to solve it.

For now I don't know if it should be considered an mpv bug, but it is a change from the previous behavior.

The behavior is the same on NetBSD (tested xterm), but unlike on Linux, typing exit at the xterm shell doesn't close the window either. But we don't have an "OS:unix" label, so leaving the label as linux for now.

commit f2c7c641b38d060a7eea52a92bc0239f1603bffb
Author: wm4 <wm4@nowhere>
Date:   Sun Feb 16 21:57:17 2020 +0100

    subprocess: implement proper detached processes on POSIX

    The previous method for this sucked: for every launched detached
    process, it started a thread, which then would leak if the launched
    process didn't end before the player uninitialized. This was very racy
    (although I bet the race condition wouldn't trigger in a 100 years), and
    wasteful (threads aren't a cheap resource).

    Implement it for POSIX directly. posix_spawn() has no direct support for
    this, so we need to do it ourselves with fork(). We could probably do it
    without fork(), and attempt to collect the PID in another thread. But
    then we'd either have a waiting thread again, or we'd need to do an
    unsafe waitpid(-1, ...) call. (POSIX process management sucks so badly,
    how did they even manage this. Hopefully I'm just missing something, but
    I'm not.) So now we depend on both posix_spawn() _and_ fork(), isn't it
    fun?

    Also call setsid(), to essentially detach the child process from the
    terminal. (Otherwise it can receive various signals from the terminal,
    which is probably not what you want.) posix_spawn() adds
    POSIX_SPAWN_SETSID in newer POSIX releases, but we don't want to rely on
    this yet.

    The posix_spawnp() call is duplicated, but this is better than somehow
    trying to unify the code paths.

    Only somewhat tested, so enjoy the bugs.
Flurrywinde commented 3 years ago

Strange, I don't remember adding the OS:linux label. It must be automatic? Anyway, thanks for looking into this. (I am "enjoying the bugs." :-) ) I'm also glad to hear that it seems it's not all spawned processes that are turned immortal. Until there's a better fix, in case it helps other people running into this same issue, here's a workaround. In my mpv-info script, I backgrounded the dzen2 process (by adding & to the end), and then after this, added the following code:

dzenpid=$!
sleep 6
dzenpids=( $(pidof dzen2) )
for p in "${dzenpids[@]}"; do
    if [ $p -eq $dzenpid ]; then
        kill -9 $dzenpid
    fi
done
Akemi commented 3 years ago

it adds it automatically based on the issue template you choose.

avih commented 3 years ago

FWIW, the behavior is the same when using the subprocess command with or without detaching. Basically, every way I tried to launch xterm from an mpv command (excluding lua's os.exec) results in xterm not closing when clicking its X close button, and I presume it would be the same for dzen2.

I'm not familiar enough with posix processes to try and change it though...

na-na-hi commented 3 months ago

Is this still a problem? I cannot reproduce anymore. dzen2 and xterm exit normally for me.