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 - Numérigraphe on 2014-04-17
Status: Rejected
Rejected by: Pedro Manuel Baeza on 2014-10-24
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/printing.py (+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 2014-04-17 Resubmit on 2014-07-02
Loïc Bellier - Numérigraphe (community) Abstain on 2014-04-18
Review via email: mp+216353@code.launchpad.net

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.

Lionel,

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

LB

review: Abstain

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

review: Needs Fixing

This project is now hosted on https://github.com/OCA/report-print-send. Please move your proposal there. This guide may help you https://github.com/OCA/maintainers-tools/wiki/How-to-move-a-Merge-Proposal-to-GitHub

review: Resubmit (moved on github)

Thanks!
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.

Githubified here:https://github.com/OCA/report-print-send/pull/9
Please reject this merge proposal on Launchpad to avoid confusion.

Unmerged revisions

13. By Numérigraphe on 2014-04-16

[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/printing.py'
2--- base_report_to_printer/printing.py 2013-12-27 18:43:25 +0000
3+++ base_report_to_printer/printing.py 2014-04-17 16:02:29 +0000
4@@ -129,23 +129,18 @@
5 thread.start()
6
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
33

Subscribers

People subscribed via source and target branches