697099 - runtests.py --testTimeOut=n does not work, runtests.py runs indefinitely

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Build Config, defect)

Description

[Dan Schaffer]

The --testTimeOut sets a limit in seconds to kill a test if it exceeds the limit.  We never got this working and we rely on buildbot to kill the run it exceeds 5m.  

In runtestBase.py we do:
p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
(out,err)=p.communicate()
- communicate blocks until the process finishes and causes runtests.py to hang if a test never ends

I think a better solution is to use polling with memory buffers and avoid communicate.
outFile=tempFile.spooledTemporaryFile()
errFile=tempFile.spooledTemporaryFile()
p = subprocess.Popen(cmd, stdout=outFile, stderr=errFile)
while True:
    if p.poll!=None:
        break
    if time()-start>self.testTimeOut:
        p.terminate()
        break
    sleep(.05) # switches to other threads
out=outFile.read()
err=errFile.read()

Comment 1

[Trevor Baker]

dupe here https://bugzilla.mozilla.org/show_bug.cgi?id=525089

Comment 2

[Dan Schaffer]

Attachment: patch

Comment 3

[Dan Schaffer]

I think I see a minor speedup because we are not using communicate() and using the in memory temp file seems fast.

If timeout occurs the code tries to call terminate() to kill the process.  The code sleeps 1s, checks if the process is still running, and retries up to 10 times.  If the script cannot kill the process maybe an exception should be thrown and the script should stop.  I'm not sure if we should bail on the test run or just keep running registering a failure.  

I should make a minor change to the patch to gracefully handle the situation.  Currently the code will thrown an exception because p.poll() returns None.  Hopefully terminate() works consistently and we can always terminiate the process.

Comment 4

[Trevor Baker]

will this also work with run-acceptance-generic-adb.sh?  how about ssh?

Comment 5

[Brent Baker]

Comment on attachment 569370 [details] [diff] [review]
patch

Review of attachment 569370 [details] [diff] [review]:
-----------------------------------------------------------------

Removing review request as this patch appears to have landed via bug 706071, tamarin-redux 6754:b442a94598f0

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.