Bug 54072 – nrwt multiprocessing: add code to handle interrupts and wedged threads

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

nrwt multiprocessing: add code to handle interrupts and wedged threads

[Dirk Pranke]

Created attachment 81761 [details]
Patch

[Dirk Pranke]

Created attachment 81763 [details]
fix missing variable assignment in _check_if_done()

[Mihai Parparita]

Comment on attachment 81763 [details]
fix missing variable assignment in _check_if_done()

View in context: https://bugs.webkit.org/attachment.cgi?id=81763&action=review

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:267
> +        return self._alive

Maybe I'm missing something, but I'm not seeing _alive being set in either this patch, the patch for bug 54071 (which this depends on), or the currently checked in code.

[Dirk Pranke]

(In reply to comment #3)
> (From update of attachment 81763 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81763&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:267
> > +        return self._alive
> 
> Maybe I'm missing something, but I'm not seeing _alive being set in either this patch, the patch for bug 54071 (which this depends on), or the currently checked in code.

Alive shows up in the patch on bug 54070 (which 54071 depends on). Sorry, a pipeline of patches can be confusing ...

-- Dirk

[Dirk Pranke]

Created attachment 82199 [details]
merge in changes from 54070, 54071, update w/ feedback from mihai, tony

[Dirk Pranke]

Created attachment 82218 [details]
merge in more changes from 54070, 54071

[Tony Chang]

Comment on attachment 82218 [details]
merge in more changes from 54070, 54071

View in context: https://bugs.webkit.org/attachment.cgi?id=82218&action=review

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:279
> +    def is_alive(self):
> +        # FIXME: We can remove this once everyone is on 2.6.
> +        return self.isAlive()

is_alive = isAlive (probably with the comment).

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:328
> +            _Multiprocessing_Process.is_alive(self)

multiprocessing.Process.is_alive

My earlier thought was if _Process/_MultiProcessWorkerConnection is the same as _Thread/_ThreadedWorkerConnection on python 2.5, maybe we shouldn't even declare this object in python 2.5 (i.e., put the whole class definition behind an if multiprocessing).  That would avoid some of these branches in the object itself.

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker_unittest.py:192
> +        oc.restore_output()

Should this be part of the try/finally?

> Tools/Scripts/webkitpy/layout_tests/layout_package/worker.py:87
> +                # FIXME: Figure out how to send a message with a traceback.

I see, the traceback can't be pickled?  Maybe we should just send back the stringified traceback?

[Dirk Pranke]

(In reply to comment #7)
> (From update of attachment 82218 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82218&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:279
> > +    def is_alive(self):
> > +        # FIXME: We can remove this once everyone is on 2.6.
> > +        return self.isAlive()
> 
> is_alive = isAlive (probably with the comment).
> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker.py:328
> > +            _Multiprocessing_Process.is_alive(self)
> 
> multiprocessing.Process.is_alive
> 
> My earlier thought was if _Process/_MultiProcessWorkerConnection is the same as _Thread/_ThreadedWorkerConnection on python 2.5, maybe we shouldn't even declare this object in python 2.5 (i.e., put the whole class definition behind an if multiprocessing).  That would avoid some of these branches in the object itself.
> 

I guess for some reason I was thinking that declaring a top-level name conditionally was a bad idea, but now I can't see any real downside, and it does make the code somewhat cleaner. Will do.

> > Tools/Scripts/webkitpy/layout_tests/layout_package/manager_worker_broker_unittest.py:192
> > +        oc.restore_output()
> 
> Should this be part of the try/finally?
>

Done.
 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/worker.py:87
> > +                # FIXME: Figure out how to send a message with a traceback.
> 
> I see, the traceback can't be pickled?  Maybe we should just send back the stringified traceback?

Correct. Directly calling str() on the traceback is useless (it just prints out the object handle), and so the FIXME is to do something like log the error to a dummy handler and then send the array of strings, if that does turn out to be useful. I'm not convinced it's worth the effort over just logging the traceback here and sending the exception w/o the traceback. Hence the fixme ... to poke into it some more at some later date when I have time.

[Dirk Pranke]

Created attachment 82359 [details]
upload w/ more feedback from tony - remove the _Multiprocessing_Process hack completely, just make the class conditionally defined

[Dirk Pranke]

Committed r78506: <http://trac.webkit.org/changeset/78506>