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