launchpad-reviewers team mailing list archive

[Stuart Bishop <stuart.bishop@xxxxxxxxxxxxx>]

Stuart Bishop has proposed merging lp:~stub/launchpad/garbo into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #768139 in Launchpad itself: "garbo script timeout behavior needs tweaking"
  https://bugs.launchpad.net/launchpad/+bug/768139
  Bug #795305 in Launchpad itself: "bugsummaryjournal is not automatically rolled up"
  https://bugs.launchpad.net/launchpad/+bug/795305
  Bug #893720 in Launchpad itself: "garbo.py not threadsafe (fails with IndexError due to race condition)"
  https://bugs.launchpad.net/launchpad/+bug/893720

For more details, see:
https://code.launchpad.net/~stub/launchpad/garbo/+merge/103430

= Summary =

garbo.py isn't thread safe, and occasionally OOPSes when a list is accessed in a non-thread safe fashion.

== Proposed fix ==

Access the list in a thread safe fashion. We could replace the list with a Queue, but that seems to increase complexity for little gain.

== Pre-implementation notes ==

== LOC Rationale ==

== Implementation details ==

== Tests ==

Existing tests pass. I'm unable to write a test to force the race condition.

== Demo and Q/A ==


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/scripts/garbo.py
-- 
https://code.launchpad.net/~stub/launchpad/garbo/+merge/103430
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stub/launchpad/garbo into lp:launchpad.

=== modified file 'lib/lp/scripts/garbo.py'
--- lib/lp/scripts/garbo.py	2012-04-02 07:10:56 +0000
+++ lib/lp/scripts/garbo.py	2012-04-25 09:21:32 +0000
@@ -1277,10 +1277,10 @@
                     threading.currentThread().name)
                 break
 
-            num_remaining_tasks = len(tunable_loops)
-            if not num_remaining_tasks:
+            try:
+                tunable_loop_class = tunable_loops.pop(0)
+            except IndexError:
                 break
-            tunable_loop_class = tunable_loops.pop(0)
 
             loop_name = tunable_loop_class.__name__
 
@@ -1315,7 +1315,7 @@
             try:
                 loop_logger.info("Running %s", loop_name)
 
-                abort_time = self.get_loop_abort_time(num_remaining_tasks)
+                abort_time = self.get_loop_abort_time(len(tunable_loops) + 1)
                 loop_logger.debug2(
                     "Task will be terminated in %0.3f seconds",
                     abort_time)