<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - run-webkit-tests should trigger a spindump when WebContent process is unresponsive"
href="https://bugs.webkit.org/show_bug.cgi?id=159827#c51">Comment # 51</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - run-webkit-tests should trigger a spindump when WebContent process is unresponsive"
href="https://bugs.webkit.org/show_bug.cgi?id=159827">bug 159827</a>
from <span class="vcard"><a class="email" href="mailto:dbates@webkit.org" title="Daniel Bates <dbates@webkit.org>"> <span class="fn">Daniel Bates</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=159827#c50">comment #50</a>)
<span class="quote">> Comment on <span class=""><a href="attachment.cgi?id=285120&action=diff" name="attach_285120" title="Patch">attachment 285120</a> <a href="attachment.cgi?id=285120&action=edit" title="Patch">[details]</a></span>
> Patch
>
> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=285120&action=review">https://bugs.webkit.org/attachment.cgi?id=285120&action=review</a>
>
> >> Tools/Scripts/webkitpy/port/ios.py:423
> >> + ], return_exit_code=True)
> >
> > Notice that Executive.run_command() only throws a ScriptError exception if return_exit_code := False. It is good programming practice to limit the scope of a try-except to the minimum code that can raise an exception to help make it straightforward to debug the cause of an exception. In <a href="show_bug.cgi?id=159827#c25">comment #25</a> I gave an example of one way we can write this code such that we limit the scope of the try-except block.
>
> This perhaps brings up another question, then, about sample_file_path(...),
> which calls filesystem.join(...). In the original sample_process code,
> finding the report was in the try catch block, which is why I kept it there
> after adding spindump. If filesystem.join(...) throws an exception (or
> results_directory(), actually), what is our desired functionality? </span >
Filesystem.join() does not raise an exception because it turns around and calls os.path.join(). And I do not expect os.path.join() to raise an exception because its documentation, <<a href="https://docs.python.org/2/library/os.path.html">https://docs.python.org/2/library/os.path.html</a>>, as of 08/02 makes no mention that it raises an exception.
<span class="quote">> I think for the time being, keeping the generation of the hang_report file path
> inside the try-except block is prudent</span >
Notice that the try-except block only catches a webkitpy.common.system.executive.ScriptError exception. That is, it does not catch a general-purpose Python Exception.
<span class="quote">> , as the tests can continue if an
> error happens while attempting to generate either version of the crash
> report, and </span >
<span class="quote">> it would seem unhelpful to be to have two try-except blocks in
> this function.
> </span >
How did you come to the conclusion that we need to try-except blocks in this function? Notice that I have exactly one try-except block in the example I gave in <a href="show_bug.cgi?id=159827#c25">comment #25</a>.
As aforementioned above, the try-except block in sample_process() only catches a webkitpy.common.system.executive.ScriptError exception. The current code allows any other exception to bubble up. Maybe we should revise this code to be more resilient to other kinds of exceptions. If so, such consideration and work is more appropriate in a separate bug as the focus of this bug is to add an enhancement to our sampling machinery: use spindump if we can.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>