<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#c53">Comment # 53</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:jbedard@apple.com" title="Jonathan Bedard <jbedard@apple.com>"> <span class="fn">Jonathan Bedard</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=159827#c51">comment #51</a>)
<span class="quote">> (In reply to <a href="show_bug.cgi?id=159827#c50">comment #50</a>)
> > 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?
>
> 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.
>
> > I think for the time being, keeping the generation of the hang_report file path
> > inside the try-except block is prudent
>
> 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.
>
> > , as the tests can continue if an
> > error happens while attempting to generate either version of the crash
> > report, and
>
> > it would seem unhelpful to be to have two try-except blocks in
> > this function.
> >
>
> 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.</span >
(In reply to <a href="show_bug.cgi?id=159827#c52">comment #52</a>)
<span class="quote">> (In reply to <a href="show_bug.cgi?id=159827#c51">comment #51</a>)
> [...]
> > > it would seem unhelpful to be to have two try-except blocks in
> > > this function.
> > >
> >
> > How did you come to the conclusion that we need to try-except blocks in this function?
>
> This should read:
>
> How did you come to the conclusion that we need two try-except blocks in
> this function?</span >
Sorry, I think my comment was not very clear.
What I was saying is that I kept the hang_report assignment in the try-except block because it had been in it's previous iteration, and I wasn't quite sure what kind of exceptions (if any) are thrown by sample_file_path(...) (as you point out, probably none, even when including results_directory()). Essentially, I was trying to keep the same structure as had existed previously, and taking sample_file_path out of the try-except would have broken that pattern.
Since it does seem like sample_file_path(...) won't be throwing any exceptions, I will change this in the re-factor.</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>