mtdean at thirdcontact
Nov 18, 2009, 10:11 PM
Post #23 of 34
(2509 views)
Permalink
|
|
Re: defunct mythfrontend children processes
[In reply to]
|
|
<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
|
mythtv-7135-defunct_processes.patch
1
