Bug 62614 – rwt: handle worker exceptions cleanly

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

[Dirk Pranke]

rwt: handle worker exceptions cleanly

[Dirk Pranke]

Created attachment 97049 [details]
Patch

[WebKit Review Bot]

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.

[Dirk Pranke]

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.

[Tony Chang]

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?

[Dirk Pranke]

(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.

[Dirk Pranke]

Created attachment 97161 [details]
update w/ review feedback from tony

[Tony Chang]

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.

[Dirk Pranke]

Committed r88848: <http://trac.webkit.org/changeset/88848>