Bug 54072 - nrwt multiprocessing: add code to handle interrupts and wedged threads
Summary: nrwt multiprocessing: add code to handle interrupts and wedged threads
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on: 54071
Blocks: 49566 54074 54082
  Show dependency treegraph
 
Reported: 2011-02-08 23:58 PST by Dirk Pranke
Modified: 2011-02-14 14:13 PST (History)
4 users (show)

See Also:


Attachments
Patch (14.56 KB, patch)
2011-02-09 00:06 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
fix missing variable assignment in _check_if_done() (14.59 KB, patch)
2011-02-09 00:24 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
merge in changes from 54070, 54071, update w/ feedback from mihai, tony (19.20 KB, patch)
2011-02-11 15:39 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
merge in more changes from 54070, 54071 (20.96 KB, patch)
2011-02-11 18:04 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
upload w/ more feedback from tony - remove the _Multiprocessing_Process hack completely, just make the class conditionally defined (22.24 KB, patch)
2011-02-14 14:09 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2011-02-08 23:58:26 PST
nrwt multiprocessing: add code to handle interrupts and wedged threads
Comment 1 Dirk Pranke 2011-02-09 00:06:28 PST
Created attachment 81761 [details]
Patch
Comment 2 Dirk Pranke 2011-02-09 00:24:30 PST
Created attachment 81763 [details]
fix missing variable assignment in _check_if_done()
Comment 3 Mihai Parparita 2011-02-11 11:58:01 PST
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.
Comment 4 Dirk Pranke 2011-02-11 12:45:08 PST
(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
Comment 5 Dirk Pranke 2011-02-11 15:39:28 PST
Created attachment 82199 [details]
merge in changes from 54070, 54071, update w/ feedback from mihai, tony
Comment 6 Dirk Pranke 2011-02-11 18:04:49 PST
Created attachment 82218 [details]
merge in more changes from 54070, 54071
Comment 7 Tony Chang 2011-02-14 10:38:18 PST
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?
Comment 8 Dirk Pranke 2011-02-14 14:00:38 PST
(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.
Comment 9 Dirk Pranke 2011-02-14 14:09:37 PST
Created attachment 82359 [details]
upload w/ more feedback from tony - remove the _Multiprocessing_Process hack completely, just make the class conditionally defined
Comment 10 Dirk Pranke 2011-02-14 14:13:24 PST
Committed r78506: <http://trac.webkit.org/changeset/78506>