From c315ee86dac996ac533b738f7c8777f4d01a0472 Mon Sep 17 00:00:00 2001 From: Jakub Stasiak Date: Wed, 11 Nov 2015 00:11:02 +0100 Subject: [PATCH] greenio: Remove sendall-like semantincs from GreenSocket.send When a socket timeout was set the sendall-like semantics was resulting in losing information about sent data and raising socket.timeout exception. The previous GreenSocket.send() implementation used to call fd.send() in a loop until all data was sent. The issue would manifest if at least one fd.send() call succeeded and was followed by a fd.send() call that would block and a trampoline that timed out. The client code would see socket.timeout being raised and would rightfully assume that no data was sent which would be incorrect. Discussed at https://github.com/eventlet/eventlet/issues/260. The original commit[1] has been reverted because it broke the test_multiple_readers test. After some debugging I believe I more or less know what's going on: The test uses sendall() which calls send() in a loop if send() reports only part of the data sent. Previously sendall() would call send() only one anyway because send() had sendall-like semantics; send has an internal loop on its own and, previously, it'd call the underlying socket object send() and, in case of partial write, call trampoline mechanism which would switch to another greenthread ready to run. After the change partial writes no longer result in trampoline mechanism being called which means that during this test it's likely that sendall() doesn't yield control to the hub even once. Similarly the current recv() implementation - it attemps to read from a socket first and only yields control to the hub if there's nothing to read at the moment; when one of the readers obtained control it's likely it'd manage to read all the data from the socket without yielding control to the hub and letting the other reader receive any data. The test changes the sending code so that it not only yields to the hub but also waits a bit so that both the readers have to yield to the hub when trying to read and there's no data available; the tests confirmed this lets both the readers receive some data from the socket which is the purpose of the test. [1] 4656eadfa5ae1237036a63ad4004dbee4572debf --- eventlet/greenio/base.py | 10 +--------- tests/greenio_test.py | 9 ++++++++- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/eventlet/greenio/base.py b/eventlet/greenio/base.py index fc9871f..634304a 100644 --- a/eventlet/greenio/base.py +++ b/eventlet/greenio/base.py @@ -359,28 +359,20 @@ class GreenSocket(object): if self.act_non_blocking: return send_method(data, *args) - # blocking socket behavior - sends all, blocks if the buffer is full - total_sent = 0 - len_data = len(data) while 1: try: - total_sent += send_method(data[total_sent:], *args) + return send_method(data, *args) except socket.error as e: eno = get_errno(e) if eno == errno.ENOTCONN or eno not in SOCKET_BLOCKING: raise - if total_sent == len_data: - break - try: self._trampoline(self.fd, write=True, timeout=self.gettimeout(), timeout_exc=socket.timeout("timed out")) except IOClosed: raise socket.error(errno.ECONNRESET, 'Connection closed by another thread') - return total_sent - def send(self, data, flags=0): return self._send_loop(self.fd.send, data, flags) diff --git a/tests/greenio_test.py b/tests/greenio_test.py index fdd2ac1..7b0ff7c 100644 --- a/tests/greenio_test.py +++ b/tests/greenio_test.py @@ -818,7 +818,14 @@ class TestGreenIoLong(tests.LimitedTestCase): client = socket.socket(socket.AF_INET, socket.SOCK_STREAM) client.connect(('127.0.0.1', listener.getsockname()[1])) bufsized(client, size=sendsize) - client.sendall(b'*' * sendsize) + + # Split into multiple chunks so that we can wait a little + # every iteration which allows both readers to queue and + # recv some data when we actually send it. + for i in range(20): + eventlet.sleep(0.001) + client.sendall(b'*' * (sendsize // 20)) + client.close() server_coro.wait() listener.close()