openerp-community-reviewer team mailing list archive

[Lionel Sausin - Numérigraphe <ls@xxxxxxxxxxxxxxxx>]

Lionel Sausin - Numérigraphe has proposed merging lp:~numerigraphe-team/report-print-send/7.0-no-lock-in-update-1308635-ls into lp:report-print-send.

Requested reviews:
  Report Printing and Sending Core Editors (report-print-send-core-editors)
Related bugs:
  Bug #1308635 in Report - Printing and Sending: "Locking seems wrong in base_report_to_printer's update()"
  https://bugs.launchpad.net/report-print-send/+bug/1308635

For more details, see:
https://code.launchpad.net/~numerigraphe-team/report-print-send/7.0-no-lock-in-update-1308635-ls/+merge/216353

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.
-- 
https://code.launchpad.net/~numerigraphe-team/report-print-send/7.0-no-lock-in-update-1308635-ls/+merge/216353
Your team Report Printing and Sending Core Editors is requested to review the proposed merge of lp:~numerigraphe-team/report-print-send/7.0-no-lock-in-update-1308635-ls into lp:report-print-send.

=== 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