Bug 62614

Summary: rwt: handle worker exceptions cleanly
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: New BugsAssignee: 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 Flags
Patch
none
update w/ review feedback from tony tony: review+

Description Dirk Pranke 2011-06-13 17:36:11 PDT
rwt: handle worker exceptions cleanly
Comment 1 Dirk Pranke 2011-06-13 17:53:09 PDT
Created attachment 97049 [details]
Patch
Comment 2 WebKit Review Bot 2011-06-13 17:56:19 PDT
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 3 Dirk Pranke 2011-06-13 17:58:48 PDT
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 4 Tony Chang 2011-06-14 09:40:28 PDT
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?
Comment 5 Dirk Pranke 2011-06-14 13:19:29 PDT
(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.
Comment 6 Dirk Pranke 2011-06-14 13:20:33 PDT
Created attachment 97161 [details]
update w/ review feedback from tony
Comment 7 Tony Chang 2011-06-14 13:30:04 PDT
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.
Comment 8 Dirk Pranke 2011-06-14 13:37:08 PDT
Committed r88848: <http://trac.webkit.org/changeset/88848>