Change in vdsm[master]: jsonrpc: Fix race in AsynChat

smizrahi at redhat.com smizrahi at redhat.com
Thu Jun 6 20:41:12 UTC 2013


Saggi Mizrahi has uploaded a new change for review.

Change subject: jsonrpc: Fix race in AsynChat
......................................................................

jsonrpc: Fix race in AsynChat

The asynchat implementation in python is riddled with bugs. This fixes
them.

Change-Id: Idfb18afa8ce7aa6e7636928922cf7c749ef67ae9
Signed-off-by: Saggi Mizrahi <smizrahi at redhat.com>
---
M lib/yajsonrpc/betterAsyncore.py
M tests/jsonRpcTests.py
2 files changed, 36 insertions(+), 41 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/29/15429/1

diff --git a/lib/yajsonrpc/betterAsyncore.py b/lib/yajsonrpc/betterAsyncore.py
index 33875f4..9782827 100644
--- a/lib/yajsonrpc/betterAsyncore.py
+++ b/lib/yajsonrpc/betterAsyncore.py
@@ -12,7 +12,7 @@
 
 
 # This is a copy of the standard library asyncore converted to support
-# compositing
+# compositing. Also fixes races in original implementation.
 class AsyncChat(object):
     # these are overridable defaults
 
@@ -152,12 +152,10 @@
                     self.producer_fifo.append(data[i:i+sabs])
             else:
                 self.producer_fifo.append(data)
-            self.initiate_send(dispatcher)
 
     def push_with_producer(self, producer, dispatcher):
         with self._fifoLock:
             self.producer_fifo.append(producer)
-            self.initiate_send(dispatcher)
 
     def readable(self, dispatcher):
         "predicate for inclusion in the readable for select()"
@@ -179,29 +177,30 @@
 
     def initiate_send(self, dispatcher):
         while self.producer_fifo and dispatcher.connected:
-            first = self.producer_fifo[0]
-            # handle empty string/buffer or None entry
-            if not first:
-                del self.producer_fifo[0]
-                if first is None:
-                    dispatcher.handle_close()
-                    return
-
-            # handle classic producer behavior
-            obs = self.ac_out_buffer_size
-            try:
-                with catch_warnings():
-                    if py3kwarning:
-                        filterwarnings("ignore", ".*buffer",
-                                       DeprecationWarning)
-                    data = buffer(first, 0, obs)
-            except TypeError:
-                data = first.more()
-                if data:
-                    self.producer_fifo.appendleft(data)
-                else:
+            with self._fifoLock:
+                first = self.producer_fifo[0]
+                # handle empty string/buffer or None entry
+                if not first:
                     del self.producer_fifo[0]
-                continue
+                    if first is None:
+                        dispatcher.handle_close()
+                        return
+
+                # handle classic producer behavior
+                obs = self.ac_out_buffer_size
+                try:
+                    with catch_warnings():
+                        if py3kwarning:
+                            filterwarnings("ignore", ".*buffer",
+                                           DeprecationWarning)
+                        data = buffer(first, 0, obs)
+                except TypeError:
+                    data = first.more()
+                    if data:
+                        self.producer_fifo.appendleft(data)
+                    else:
+                        del self.producer_fifo[0]
+                    continue
 
             # send the data
             try:
@@ -210,13 +209,14 @@
                 dispatcher.handle_error()
                 return
 
-            if num_sent:
-                if num_sent < len(data) or obs < len(first):
-                    self.producer_fifo[0] = first[num_sent:]
-                else:
-                    del self.producer_fifo[0]
-            # we tried to send some actual data
-            return
+            with self._fifoLock:
+                if num_sent:
+                    if num_sent < len(data) or obs < len(first):
+                        self.producer_fifo[0] = first[num_sent:]
+                    else:
+                        del self.producer_fifo[0]
+                # we tried to send some actual data
+                return
 
     def discard_buffers(self):
         # Emergencies only!
diff --git a/tests/jsonRpcTests.py b/tests/jsonRpcTests.py
index a345eea..e7954c0 100644
--- a/tests/jsonRpcTests.py
+++ b/tests/jsonRpcTests.py
@@ -22,7 +22,6 @@
 import logging
 from Queue import Queue
 from contextlib import contextmanager
-from testValidation import brokentest
 
 from testrunner import VdsmTestCase as TestCaseBase, \
     expandPermutations, \
@@ -62,11 +61,9 @@
         while True:
             try:
                 client, msg = self._queue.get()
-                self.log.info("Echoing message A")
                 if client is None:
                     return
 
-                self.log.info("Echoing message")
                 client.send(msg)
             except Exception:
                 self.log.error("EchoServer died unexpectedly", exc_info=True)
@@ -91,9 +88,6 @@
 
 @expandPermutations
 class ReactorTests(TestCaseBase):
-    @brokentest("We sometime see: "
-                "AssertionError: Data is not as expected "
-                "'Lorem ipsu...o eiusmod ' != 'Lorem ipsu... eiusmod t'")
     @permutations(CONNECTION_PERMUTATIONS)
     def test(self, rt, ssl):
         data = dummyTextGenerator(((2 ** 10) * 200))
@@ -144,12 +138,13 @@
                         self.log.info("Asserting reply...")
                         self.assertTrue(isinstance(retData,
                                                    (str, unicode)))
+                        plen = 20  # Preview len, used for debugging
                         self.assertEquals(
                             retData, data,
                             "Data is not as expected " +
-                            "'%s...%s' != '%s...%s'" %
-                            (retData[:10], retData[-10:],
-                             data[:10], data[-10:]))
+                            "'%s...%s' (%d chars) != '%s...%s' (%d chars)" %
+                            (retData[:plen], retData[-plen:], len(retData),
+                             data[:plen], data[-plen:], len(data)))
             finally:
                 queue.put((None, None))
 


--
To view, visit http://gerrit.ovirt.org/15429
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Idfb18afa8ce7aa6e7636928922cf7c749ef67ae9
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi at redhat.com>


More information about the vdsm-patches mailing list