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

[Lionel Sausin - Initiatives/Numérigraphe]

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.

[Loïc Bellier - Numérigraphe (lb-b)]

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

[Yannick Vaucher @ Camptocamp (yvaucher-c2c)]

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

review: Needs Fixing

[Yannick Vaucher @ Camptocamp (yvaucher-c2c)]

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: Needs Resubmitting (moved on github)

[Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives)]

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.

[Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives)]

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

=== modified file 'base_report_to_printer/printing.py'
--- base_report_to_printer/printing.py	2013-12-27 18:43:25 +0000
+++ base_report_to_printer/printing.py	2014-04-17 16:02:29 +0000
@@ -129,23 +129,18 @@
         thread.start()
 
     def update(self, cr, uid, context=None):
-        if not context or context.get('skip_update'):
-            return
-        self.lock.acquire()
+        """Update printer status if current status is more than 10s old."""
+        # We won't acquire locks - we're only assigning from immutable data
+        if not context or 'skip_update' in context:
+            return True
         last_update = self.last_update
-        updating = self.updating
-        self.lock.release()
         now = time.time()
-        # Only update printer status if current status is more than 10 seconds old.
         if not last_update or now - last_update > 10:
             self.start_printer_update(cr, uid, context)
             # Wait up to five seconds for printer status update
-            for x in range(0,5):
+            for _ in range(0, 5):
                 time.sleep(1)
-                self.lock.acquire()
-                updating = self.updating
-                self.lock.release()
-                if not updating:
+                if not self.updating:
                     break
         return True