Permalink
Browse files

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 #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] 4656ead
  • Loading branch information...
jstasiak committed Nov 10, 2015
1 parent 5c5025f commit c315ee86dac996ac533b738f7c8777f4d01a0472
Showing with 9 additions and 10 deletions.
  1. +1 −9 eventlet/greenio/base.py
  2. +8 −1 tests/greenio_test.py
View
@@ -359,28 +359,20 @@ def _send_loop(self, send_method, data, *args):
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)
View
@@ -818,7 +818,14 @@ def server():
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()

0 comments on commit c315ee8

Please sign in to comment.