[webkitcorepy] Attempt to terminate stuck processes before killing them

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

I encountered a case today where simply killing a stuck process was not sufficient, the process needed to be terminated. In order for us to trust webkitcorepy.run, we should terminate processes when they time out.

[Radar WebKit Bug Importer]

<rdar://problem/70222803>

[Jonathan Bedard]

Created attachment 411153 [details]
Patch

Comment on attachment 411153 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411153&action=review

> Tools/ChangeLog:11
> +        (run): Terminate stuck processes instead of killing them since termination is guaranteed to work.

Are we sure about this? Because `terminate` only sends SIGTERM, but `kill` sends SIGKILL, which is more powerful than the former.
Per https://docs.python.org/3/library/subprocess.html#subprocess.Popen.terminate
& https://www.gnu.org/software/libc/manual/html_node/Termination-Signals.html

[Jonathan Bedard]

(In reply to dewei_zhu from comment #3)
> Comment on attachment 411153 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411153&action=review
> 
> > Tools/ChangeLog:11
> > +        (run): Terminate stuck processes instead of killing them since termination is guaranteed to work.
> 
> Are we sure about this? Because `terminate` only sends SIGTERM, but `kill`
> sends SIGKILL, which is more powerful than the former.
> Per
> https://docs.python.org/3/library/subprocess.html#subprocess.Popen.terminate
> & https://www.gnu.org/software/libc/manual/html_node/Termination-Signals.html

Oops! You're right, it's the communicate that's our issue

[Jonathan Bedard]

Created attachment 411163 [details]
Patch

[Jonathan Bedard]

To be a bit more specific, this was found with `git svn` being run from a repository that doesn't support it.

Comment on attachment 411163 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411163&action=review

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/subprocess_utils.py:137
>              process.wait()

Not sure if this is related to the initial problem this patch intends to address.
Would this cause deadlock when
1. timeout is specified &
2. huge amount of output is produced to subprocess.PIPE
3. program hits the timeout
Per the warning from https://docs.python.org/2.7/library/subprocess.html#subprocess.Popen.wait

[Jonathan Bedard]

Comment on attachment 411163 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411163&action=review

>> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/subprocess_utils.py:137
>>              process.wait()
> 
> Not sure if this is related to the initial problem this patch intends to address.
> Would this cause deadlock when
> 1. timeout is specified &
> 2. huge amount of output is produced to subprocess.PIPE
> 3. program hits the timeout
> Per the warning from https://docs.python.org/2.7/library/subprocess.html#subprocess.Popen.wait

We shouldn't have to worry about that case because we've already killed the process and attempted to communicate....actually, I suspect that we run a risk of hanging a program in that case on line 129, but it shouldn't be a deadlock, it should just be until we consume the buffer. That being said, the existing code had the same problem, so I think we should defer such a fix until we have evidence we actually need it.

[EWS]

Committed r268373: <https://trac.webkit.org/changeset/268373>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411163 [details].