Merge lp:~stub/launchpad/garbo into lp:launchpad

Proposed by Stuart Bishop on 2012-04-25
Status: Merged
Approved by: Curtis Hovey on 2012-04-25
Approved revision: 8828
Merged at revision: 15151
Proposed branch: lp:~stub/launchpad/garbo
Merge into: lp:launchpad
Diff against target: 29 lines (+7/-4) 1 file modified
To merge this branch: bzr merge lp:~stub/launchpad/garbo
Reviewer Review Type Date Requested Status
Curtis Hovey code 2012-04-25 Approve on 2012-04-25
Review via email: mp+103430@code.launchpad.net

Description of the Change

= 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

To post a comment you must log in.
Curtis Hovey (sinzui) wrote :

Hi Stuart.

I like your solution. I looked at the bug too and pondered if a try/except would be easier to maintain than a queue. Can you add a comment before the break to explain catching the IndexError is safer than looking at the length because the work is threaded?

review: Approve (code)
Stuart Bishop (stub) wrote :

I've added the comment. Thanks!

lp:~stub/launchpad/garbo updated on 2012-04-25
8829. By Stuart Bishop on 2012-04-25

Comment

Preview Diff

1=== modified file 'lib/lp/scripts/garbo.py'
2--- lib/lp/scripts/garbo.py 2012-04-02 07:10:56 +0000
3+++ lib/lp/scripts/garbo.py 2012-04-25 14:27:41 +0000
4@@ -1277,10 +1277,13 @@
5 threading.currentThread().name)
6 break
7
8- num_remaining_tasks = len(tunable_loops)
9- if not num_remaining_tasks:
10+ try:
11+ tunable_loop_class = tunable_loops.pop(0)
12+ except IndexError:
13+ # We catch the exception rather than checking the
14+ # length first to avoid race conditions with other
15+ # threads.
16 break
17- tunable_loop_class = tunable_loops.pop(0)
18
19 loop_name = tunable_loop_class.__name__
20
21@@ -1315,7 +1318,7 @@
22 try:
23 loop_logger.info("Running %s", loop_name)
24
25- abort_time = self.get_loop_abort_time(num_remaining_tasks)
26+ abort_time = self.get_loop_abort_time(len(tunable_loops) + 1)
27 loop_logger.debug2(
28 "Task will be terminated in %0.3f seconds",
29 abort_time)