[Bug 260719] Vlc hangs on open file dialog

[Rémi Denis-Courmont]

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





--- Comment #48 from Rémi Denis-Courmont <remi remlab net>  2011-02-03 09:15:22 ---
(In reply to comment #46)
> (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.

Oh yes. But that only works (safely) in single-threaded processes. Otherwise,
chaining signal handlers is more likely to crash than anything.

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

Exactly. And that you cannot promise in a multithreaded context like VLC.

> > 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.

No. There cannot, not in a multithread context. So it only safely works as long
as ONLY Qt is does this, which makes Qt not cooperative.

> > 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.

It's not thread-safe, it's conditionally thread-safe which is not thread-safe.
That's a lying.

If it were thread-safe, this bug would not exist in the first place.

> 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.

The cost of thread, especially if the stack size is kept low, is peanuts to the
cost of forking that QProcess fundamentally does.

> 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.

That's not correct; waitpid() is a cancellation point. You just need to make
sure the thread entry point has C linkage and no C++ objects.

But no matter what you do (SIGCHLD, waitpid, etc), "stop watching" will leak
the zombie process until your own process exits, which is a rather bad idea. If
you really want to abort a subprocess, it's generally a better idea to close
its input pipe, or more radically to close its output pipe and/or kill() it.

Eventually, you'll need to wait for waitpid() anyway; you just have better
chances that it won't block.

(...)
> 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.

Unlike my solution, it fails miserably in thread-aware processes like VLC that
use sigwait() for synchronous signal delivery.

> 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. 

It is not thread-safe and therefore not cooperative. Please stop the lies.

> 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.

That won't even work.

Contrary to your statement, VLC does not even install any signal handler on
SIGCHLD. It just restores once SIG_DFL at the very beginning of main() in case
the parent process had set SIG_IGN. Qt is initialized much much later. 

Contrarily also, VLC does not block SIGCHLD. Otherwise waitpid() wouldn't work
as far as I know. It blocks it in all thread except the main() one to avoid
unintended EINTR throughout the code base.

> 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.

No, you are starting with the wrong assumption that VLC has a handler and
removes Qt's own. It has *none* and it does not remove any.

The only signal handler VLC has is SIG_IGN for SIGPIPE for obvious reasons.

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