<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#c26">Comment # 26</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>Comment on <span class=""><a href="attachment.cgi?id=284354&action=diff" name="attach_284354" title="Patch">attachment 284354</a> <a href="attachment.cgi?id=284354&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=284354&action=review">https://bugs.webkit.org/attachment.cgi?id=284354&action=review</a>
<span class="quote">>> Tools/WebKitTestRunner/TestInvocation.cpp:203
>> + return;
>
> OK. Is it necessary to know whether we are running in server mode to know if we should prompt for input for sample completion before terminating the WebContent process? I suspect that waiting for a sample to be taken of an unresponsive WebContent process is mostly of interest to scripts such as run-webkit-tests (for the purpose of implementing it sample on timeout feature). Would it be sufficient to only prompt to sample the WebContent process if standard error is not attached to a tty (e.g. isatty(fileno(stderr)) returns 0)?</span >
Since it is legal to have a script driving the testing process and not be in server mode (even if this may not be a current use case) I figure that it is safer to check for server mode. We will never waiting for standard in if we aren't in server mode, so while it may not be needed for current use cases, it seems prudent so that this code doesn't cause hangs later on.
<span class="quote">>> Tools/WebKitTestRunner/TestInvocation.cpp:206
>> + if (isatty(fileno(stdin)))
>
> This does not seem correct. We should be checking whether standard error is a tty device as opposed to standard input because scripts that tend to capture standard output and standard error of a subprocess (like run-webkit-tests) tend to forward standard input to the subprocess.</span >
Well, it is possible for a user to pipe standard error to a file, correct? It seems to me that in this circumstance, where the user pipes standard error into a file by standard in is a terminal, we should still prompt the user, as the application will hang, but at least standard error will have a line describing why.
<span class="quote">>> Tools/WebKitTestRunner/TestInvocation.cpp:213
>> + fputs("Failed to sample process\n", stderr);
>
> Maybe we could simplify the above logic to be:
>
> ...
> dump(errorMessage, buffer, true);
>
> if (!isatty(fileno(stderr))) {
> // Run-webkit-tests may have initiated a process sample of the unresponsive process. We wait for run-webkit-tests to
> // signal completion of its sample activity (it will signal regardless of whether it actually takes a process sample)
> // before we continue on and terminate the unresponsive process.
> if (!fgets(buffer, sizeof(buffer), stdin) || strcmp(buffer, "#SAMPLE FINISHED\n"))
> fprintf(stderr, "Failed to receive expected sample finished response. Got \"%s\". Continuing...", buffer);
> }</span >
The way I see this, we have a number of use cases:
Not in server mode: Probably not being run by a script, no stdin expected
Server mode, stdin a terminal, stderr and stdout to pipes: A user has instantiated server mode and is piping the results, stdin expected
Server mode, stdin a pipe, stderr and stdout to pipes: Probably a script. Stdin again expected
Server mode, stdin a pipe, stderr and stdout to terminals: A user is piping in commands (so stdin expected) but reading the output
I can't think of anyone in their right mind who would use the last use case, I supposed we might should output a line to stderr if either stdin or stderr are terminals</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>