Merge lp:~roadmr/checkbox/827859 into lp:checkbox

[Daniel Manrique]

This code guards against the situation encountered in bug 827859, where a bogus timeout value can cause the backend to crash. I suspect it may have been caused by the user creating a job with a strange timeout value, but it's good to handle this in the code I think.

I'm turning all strange timeout values to zero, which in practice causes jobs with bad timeouts to outright exit as they don't complete "before" the timeout, but I think this is more desirable than a crashing backend. A possible refinement would be a default (a few seconds) timeout, but I'm not sure it's worth it on safety-net code like this.

Also if you can think of a better way to handle this (this is a bit verbose at 8 lines of code) it would be nice :)

[Brendan Donegan (brendan-donegan)]

In my opinion it might be better to default to something sensible rather than 0 for the timeout (30 seconds?), I haven't seen an example of a bogus timeout so I'm assuming the would fall under the category of 'programming error'?

Also, I don't think it's really worth changing the code for (as it reads just fine), but this would be shorter (only barely!):

if timeout is not None:
    try:
        timeout = float(timeout)
        if timeout < 0: timeout = 0
    except:
        timeout = 0 # This could maybe be 30 as a sensible default

review: Needs Information

[lp:~roadmr/checkbox/827859]

1012. By Daniel Manrique on 2011-09-09

merged from trunk

1013. By Daniel Manrique on 2011-09-09

use a default timeout instead of 0 when a job specifies invalid timeout

[Daniel Manrique (roadmr)]

Thanks, I was also unsure about the 0 seconds thing, I added a default timeout of 30 seconds.

Turns out the bogus timeout stems from bug 833747 which results in invalid job descriptions, potentially also if a user uses a timeout: value of negative or a string (timeout: forever). Still I think this bit of code is worth having.

review: Needs Resubmitting

[Brendan Donegan (brendan-donegan)]

Approved.

review: Approve

=== modified file 'checkbox/job.py'
--- checkbox/job.py	2010-04-20 16:23:16 +0000
+++ checkbox/job.py	2011-09-09 14:47:31 +0000
@@ -37,6 +37,7 @@
 
 ALL_STATUS = [FAIL, PASS, UNINITIATED, UNRESOLVED, UNSUPPORTED, UNTESTED]
 
+DEFAULT_JOB_TIMEOUT = 30 #used in case a job specifies invalid timeout
 
 class Job(object):
 
@@ -47,6 +48,14 @@
         self.command = command
         self.environ = environ
         self.timeout = timeout
+        if self.timeout is not None:
+            try:
+                self.timeout = float(self.timeout)
+            except:
+                self.timeout =DEFAULT_JOB_TIMEOUT
+            finally:
+                if self.timeout < 0:
+                    self.timeout = DEFAULT_JOB_TIMEOUT
 
     def execute(self):
         # Sanitize environment

=== modified file 'debian/changelog'
--- debian/changelog	2011-09-09 13:43:17 +0000
+++ debian/changelog	2011-09-09 14:47:31 +0000
@@ -11,6 +11,9 @@
   * Several corrections necessary due to test name changes or typos found in
     job files
 
+  [Daniel Manrique]
+  * checkbox/job.py: Guard against bogus timeout values (LP: #827859)
+
  -- Daniel Manrique <[e-mail]>  Thu, 08 Sep 2011 14:53:55 -0400
 
 checkbox (0.12.6) oneiric; urgency=low