Summary: | rwt: handle worker exceptions cleanly | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Pranke <dpranke> | ||||||
Component: | New Bugs | Assignee: | Dirk Pranke <dpranke> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, eric, ojan, tony, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Dirk Pranke
2011-06-13 17:36:11 PDT
Created attachment 97049 [details]
Patch
Attachment 97049 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1
Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:1323: deprecated form of raising exception [pep8/W602] [5]
Total errors found: 1 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 97049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97049&action=review >> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:1323 >> + raise exception_type, exception_value > > deprecated form of raising exception [pep8/W602] [5] The code is correct, and I don't know if there's a way to suppress this warning given whatever lint tool we're using, or if there's another way to re-raise this error, but otherwise this warning can be ignored, I think. Comment on attachment 97049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97049&action=review > Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:695 > + except WorkerException, e: Nit: e looks unused, maybe remove it or re-raise it? > Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:702 > raise Should we re-raise the exception here too? >>> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:1323 >>> + raise exception_type, exception_value >> >> deprecated form of raising exception [pep8/W602] [5] > > The code is correct, and I don't know if there's a way to suppress this warning given whatever lint tool we're using, or if there's another way to re-raise this error, but otherwise this warning can be ignored, I think. Can you do raise exception_type(exception_value) or does that not always work? > Tools/Scripts/webkitpy/layout_tests/layout_package/worker.py:108 > _log.debug("%s done with message loop%s" % (self._name, exception_msg)) Nit: Should the prefix ", " be moved into this log message and removed from exception_msg? (In reply to comment #4) > (From update of attachment 97049 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97049&action=review > > > Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:695 > > + except WorkerException, e: > > Nit: e looks unused, maybe remove it or re-raise it? > I've removed the "e". > > Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:702 > > raise > > Should we re-raise the exception here too? > I'm confused, what do you mean. Doesn't "raise" by itself re-raise the exception? > >>> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:1323 > >>> + raise exception_type, exception_value > >> > >> deprecated form of raising exception [pep8/W602] [5] > > > > The code is correct, and I don't know if there's a way to suppress this warning given whatever lint tool we're using, or if there's another way to re-raise this error, but otherwise this warning can be ignored, I think. > > Can you do raise exception_type(exception_value) or does that not always work? That seems to work, and doesn't get a complaint from lint. Fortunately, there's no traceback. > > > Tools/Scripts/webkitpy/layout_tests/layout_package/worker.py:108 > > _log.debug("%s done with message loop%s" % (self._name, exception_msg)) > > Nit: Should the prefix ", " be moved into this log message and removed from exception_msg? No, because exception_msg is empty if there's no exception, and we would be left with a comma but nothing following it. Created attachment 97161 [details]
update w/ review feedback from tony
Comment on attachment 97161 [details] update w/ review feedback from tony View in context: https://bugs.webkit.org/attachment.cgi?id=97161&action=review > Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:702 > raise Yes, you're right, this gives the right behavior. I was confused. If you were to raise e, the caught exception, it would lose the stack. Committed r88848: <http://trac.webkit.org/changeset/88848> |