[mythtv-users] defunct mythfrontend children processes

[Michael T. Dean]

<fixed top posting>

On 11/18/2009 10:17 PM, Johnny Walker wrote:
> On Wed, Nov 18, 2009 at 3:42 PM, Johnny Walker wrote:
>   
>> On Wed, Nov 18, 2009 at 3:37 PM, Michael T. Dean wrote:
>>     
>>> If you're using the patch I had posted to #7134, yeah, kill it.  It won't
>>> help (based on what I now think is happening).
>>>
>>> If you're using Bill's udev.c, it should fix it, but it's really a fix for
>>> #6137, not #7135.  Fixing #6137 would obviate the need for the code that
>>> causes #7135--meaning you won't see the issue if you use that approach.
>>>
>>> However, testing the patch I attached to my last message would be very
>>> useful since it's likely that a fix like that will be used for 0.22-fixes
>>> and the fix for #6137 will only go into trunk.
>> I've stopped the compile, reverted the changes, applied the patch and
>> i'm starting the compile again.
> Ok - took me a minute to get it installed and working.
>
> The bad news is that the patched compiled version of myth seems worse.
> Where as before I had only 2 or 3 defunct processes now I'm seeing 4.
>
>  4391 tty1     Sl     0:21              \_ /usr/local/bin/mythfrontend
>  4435 tty1     Z      0:00                  \_ [mythfrontend] <defunct>
>  4437 tty1     Z      0:00                  \_ [mythfrontend] <defunct>
>  4439 tty1     Z      0:00                  \_ [mythfrontend] <defunct>
>  4440 tty1     Z      0:00                  \_ [mythfrontend] <defunct>
>
> Any other patches you'd like me to try out now that I've got the hang of this?

First, thanks a lot for testing these patches, Johnny and Bill.  I still 
can't even artificially reproduce the issue on my system (lack of media 
devices, still have udevinfo, can't figure out how to cause it to fail 
the way it does for you all).

Anyway, I wasn't too surprised to see that the patch didn't help (as I 
took a quick "hope it works itself out" solution instead of a 
well-designed solution).  Then, I was making the follow-up patch and 
stumbled across http://svn.mythtv.org/trac/ticket/7135#comment:19 which 
really confused me.

Anyway, the attached patch, 
mythtv-7135-close_qprocess_before_return.patch , is probably cleaner, 
anyway, but probably needs some TLC from someone willing to experiment.

Basically, I /think/ the problem is that the QTextStream on the stack 
that's reading from the QProcess on the heap is deleting the QProcess 
before the QProcess is ready to be deleted.  I was hoping that the 
patch--which basically just moved the QProcess to the stack with the 
QTextStream (the patch that seems to have worked for Bill, but not for 
Johnny)--would help, but it may still be leaving the timing up to chance.

Attached is a different patch which actually calls the new-in-Qt4 
QProcess::close() function to close the process before we return.  The 
close() function actually calls kill() and waitForFinished(-1), so it 
should cleanly close the QProcess.  The downside is that with 
waitForFinished(-1), the wait will never time out, but the upside is the 
kill() /should/ ensure that it dies very soon...

There is still an issue with the patch.  The code block:

    if (ret.startsWith("device not found in database"))
        return ret;

on line 262 seems to be a) leaking a QProcess and b) (like the other 
code was,) not close()'ing the QProcess and c) other than causing 
leaks/problems and suppressing a log message, not really doing anything 
different than it would do were we to remove the code.  The first patch 
fixes b), and I added a second patch, 
mythtv-7135-remove_dev_not_found_check.patch , that just removes the 
check above.  Apply mythtv-7135-remove_dev_not_found_check.patch on top 
of the other patch (i.e. don't revert 
mythtv-7135-close_qprocess_before_return.patch before applying 
mythtv-7135-remove_dev_not_found_check.patch ).

If mythtv-7135-close_qprocess_before_return.patch doesn't work, we'll 
probably have to take the opposite approach that my last post to the 
list took.  Instead of moving the QProcess to the stack with the 
QTextStream, we'd need to move the QTextStream to the heap with the 
QProcess.  Then, we'd need to ensure we close() the QProcess before 
calling deleteLater() on it.  Then we'd need to call deleteLater() on 
the QTextStream.  That should ensure everything is cleaned up in the 
proper order.

Since that's ugly, though, I'm hoping we can do it with this approach 
(and/or a combination of this approach and the one that moves the 
QProcess to the stack--as it's probably cleaner than using the 
deleteLater() stuff).  I'd appreciate your testing 
mythtv-7135-close_qprocess_before_return.patch applied to a clean 
mediamonitor-unix.cpp (with none of my previous patches applied) and 
letting me know what happens.  And, if you're feeling really motivated, 
testing mythtv-7135-remove_dev_not_found_check.patch separately would be 
extra nice.

Oh, and these patches are completely untested--not even compile 
tested--as my dev system is unavailable because it's occupied on another 
project for a bit.

Thanks a lot for the testing.

Mike
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: mythtv-7135-close_qprocess_before_return.patch
URL: <http://mythtv.org/pipermail/mythtv-users/attachments/20091119/b4609238/attachment.txt>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: mythtv-7135-remove_dev_not_found_check.patch
URL: <http://mythtv.org/pipermail/mythtv-users/attachments/20091119/b4609238/attachment.asc>