Merge lp:~numerigraphe-team/report-print-send/7.0-no-lock-in-update-1308635-ls into lp:~report-print-send-core-editors/report-print-send/7.0

Proposed by Lionel Sausin - Initiatives/Numérigraphe
Status: Rejected
Rejected by: Pedro Manuel Baeza
Proposed branch: lp:~numerigraphe-team/report-print-send/7.0-no-lock-in-update-1308635-ls
Merge into: lp:~report-print-send-core-editors/report-print-send/7.0
Diff against target: 32 lines (+6/-11)
1 file modified
base_report_to_printer/ (+6/-11)
To merge this branch: bzr merge lp:~numerigraphe-team/report-print-send/7.0-no-lock-in-update-1308635-ls
Reviewer Review Type Date Requested Status
Yannick Vaucher @ Camptocamp moved on github Needs Resubmitting
Loïc Bellier - Numérigraphe (community) Abstain
Review via email:

Description of the change

If I'm not mistaken, locking in printer.printer.update() is not needed, and the way it's written does not protect us from concurrent changes anyway :
- if what we read is immutable than the value is copied and locking is unnecessary
- if it's mutable then we're getting a "reference" to the shared object, and should keep the lock until we're completely done with it

From my quick code survey, the variables seem to contain a boolean and a float so I guess no locking is needed.

update() seems to be called pretty often (all the CRUD method call it) so It may help a bit to remove the contention.

To post a comment you must log in.
Revision history for this message
Loïc Bellier - Numérigraphe (lb-b) wrote :


Maybe you can have a conflict with line "for _ in range(0, 5):" on line 24
if the import " import _" has been added ?


review: Abstain
Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote :

LGTM appart for the variable _ though we are not using translation here.

review: Needs Fixing
Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote :

This project is now hosted on Please move your proposal there. This guide may help you

review: Needs Resubmitting (moved on github)
Revision history for this message
Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives) wrote :

For "_" I tried to express in a pythonic way that the variable is in
fact unused, but clearly this is not explicit enough.
I'll change to "_unused" and re-submit on github.

Revision history for this message
Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives) wrote :

Githubified here:
Please reject this merge proposal on Launchpad to avoid confusion.

Unmerged revisions

13. By Numérigraphe

[IMP] remove useless locking from update(), shouldn't be needed for immutable data

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'base_report_to_printer/'
2--- base_report_to_printer/ 2013-12-27 18:43:25 +0000
3+++ base_report_to_printer/ 2014-04-17 16:02:29 +0000
4@@ -129,23 +129,18 @@
5 thread.start()
7 def update(self, cr, uid, context=None):
8- if not context or context.get('skip_update'):
9- return
10- self.lock.acquire()
11+ """Update printer status if current status is more than 10s old."""
12+ # We won't acquire locks - we're only assigning from immutable data
13+ if not context or 'skip_update' in context:
14+ return True
15 last_update = self.last_update
16- updating = self.updating
17- self.lock.release()
18 now = time.time()
19- # Only update printer status if current status is more than 10 seconds old.
20 if not last_update or now - last_update > 10:
21 self.start_printer_update(cr, uid, context)
22 # Wait up to five seconds for printer status update
23- for x in range(0,5):
24+ for _ in range(0, 5):
25 time.sleep(1)
26- self.lock.acquire()
27- updating = self.updating
28- self.lock.release()
29- if not updating:
30+ if not self.updating:
31 break
32 return True