[Bug 260719] Vlc hangs on open file dialog

[Thiago Macieira]

https://bugs.kde.org/show_bug.cgi?id=260719


Thiago Macieira <thiago at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |thiago at kde.org




--- Comment #46 from Thiago Macieira <thiago kde org>  2011-02-02 23:16:21 ---
(In reply to comment #21)
> In my opinion, the real problem is that Qt QProcess relies on a signal handler
> to watch for process exit. This is not cooperative... 

Sure it is. Anyone can install a SIGCHLD handler. They just need to invoke the
other handlers in the chain when they are invoked.

The only problem is that you can't remove a SIGCHLD handler, unless you do it
in perfect LIFO order.

> VLC has multiple threads
> in different components, that do create child processes. Yet there can be only
> one signal handler for SIGCHLD at a time and it will catch all process
> terminations.

That's where you're wrong. There can be multiple SIGCHLD handlers, like I
explained above.

> The only portable thread-cooperative mechanism to be notified of the
> termination of a child process terminate is more or less:
> 
>     while (waitpid(pid, 0, &status) != pid);
> 
> Indeed, this is how VLC's own code waits for child processes.

You're again wrong, because Qt has a cooperative, efficient and thread-safe
implementation. I'll explain it in a second.

The above code has two problems: it blocks (so no select/poll-based loop
integration) and it waits for one single PID.

> Obviously, this would be difficult to integrate with a poll/select main loop.
> Then, something like this would be needed:
>  - create a pipe,
>  - create a thread (write end of the pipe and child process PID as parameters),
>  - register the read end of the pipe to the main loop,
>  - thread: call waitpid() from the thread entry,
>  - thread: write the process exit status to the pipe,
>  - main loop wakes up,
>  - read the exit status from the pipe,
>  - destroy the pipe and the thread.
> This is admittedly not very straight forward nor very elegant. But I cannot
> think of any simpler yet safe approach.

I can.

Your solution is to start one thread per PID we want to watch. That's probably
thread-safe but extremely wasteful. What's more, there's no way of notifying
the watcher thread to stop watching. This probably will lead to hangs-at-exit
because not all threads have exited.

> Nevertheless as far as this particular bug is concerned, VLC freezes. That
> means the KDE code is waiting for the process synchronously. So there should be
> no need to integrate with the main loop.

There is. 

The reason why there's a loop integration is that the particular code is trying
to read from the pipes connected to the child process. To do that, it waits on
a select(2) with three pipes enabled: the two connected to the subprocess's
stdout and stderr and a third pipe which notifies that the process has exited.

When a subprocess exits, the stdout and stderr pipes get closed, so we don't
need to wait for them anymore. However, the pipes can be closed before the
process exits, so they are not an indication that it has happened.

The third pipe's other end is connected to a thread which will do more or less
what you described: it will waitpid on the PID and write a byte if it has
successfully reaped the subprocess.

However, unlike your solution, there's only one such thread running and it uses
the WNOHANG flag when calling waitpid(2). It checks all the PIDs launched from
QProcess. That is is efficient and thread-safe.

All Qt needs is to know when to try and reap the processes. It needs an
indication that a child process has exited. That's where SIGCHLD comes in.

The Qt system is efficient, correct, cooperative and thread-safe. It works on
all Unix systems with purely cross-platform and entirely POSIX-compliant code.
I will *NOT* change it unless it's for something better -- and there's no such
thing today. 

The only thing I may do to "fix" this issue is to reinstall the SIGCHLD handler
and unblock the signal -- that is, undo the damage that VLC did.

I would rather that VLC corrected its usage of SIGCHLD. You're starting from
the wrong assumption that there can be only one handler. If you need help in
fixing this issue or understanding more how to do it and how to cooperate with
Qt's handler, please contact me directly by email.

PS: the glib code for gspawn does the *exact* same thing as Qt does, except
that it forgets to chain to the previous SIGCHLD handler. I'll prepare a patch
and submit to them.

-- 
Configure bugmail: https://bugs.kde.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.