homepage

Dear,
del deque[0] is not safe, see the attached patch for the asynchat.async_chat.initiate_send method.
fix the "IndexError: deque index out of range" of "del self.producer_fifo[0]"
Best,
Pierrick Koch

The attached script, test_initiate_send.py, tests initiate_send with
threads, causing duplicate writes and an IndexError. This is the
script output using python on the default branch:
$ python test_initiate_send.py
--- Test: duplicate data sent ---
chat.send('thread data')
chat.send('thread data')
--- Test: IndexError ---
chat.send('thread data')
chat.send('thread data')
Exception in thread Thread-2:
Traceback (most recent call last):
 File "Lib/threading.py", line 644, in _bootstrap_inner
 self.run()
 File "Lib/threading.py", line 601, in run
 self._target(*self._args, **self._kwargs)
 File "test_initiate_send.py", line 25, in <lambda>
 thread = threading.Thread(target=lambda : chat.push('thread data'))
 File "Lib/asynchat.py", line 194, in push
 self.initiate_send()
 File "Lib/asynchat.py", line 254, in initiate_send
 del self.producer_fifo[0]
IndexError: deque index out of range
The script does not fail with Pierrick patch:
$ python test_initiate_send.py
--- Test: duplicate data sent ---
chat.send('main data')
chat.send('thread data')
--- Test: IndexError ---
chat.send('thread data')
The patch misses to also appendleft() 'first' when 'num_sent' is zero,
which may happen on getting EWOULDBLOCK on send().

Looks like a reasonable fix to me. What needs to be done to get this integrated?

After applying the patch I get these 2 failures on Linux:
======================================================================
FAIL: test_simple_producer (__main__.TestAsynchat)
----------------------------------------------------------------------
Traceback (most recent call last):
 File "Lib/test/test_asynchat.py", line 198, in test_simple_producer
 self.fail("join() timed out")
AssertionError: join() timed out
======================================================================
FAIL: test_simple_producer (__main__.TestAsynchat_WithPoll)
----------------------------------------------------------------------
Traceback (most recent call last):
 File "Lib/test/test_asynchat.py", line 198, in test_simple_producer
 self.fail("join() timed out")
AssertionError: join() timed out

The problem is that when the fifo contains a producer and the more()
method of the producer returns a non-empty bytes sequence, then the
producer must be put back in the fifo first. This is what does the
following change made to Pierrick patch:
diff --git a/Lib/asynchat.py b/Lib/asynchat.py
--- a/Lib/asynchat.py
+++ b/Lib/asynchat.py
@@ -229,6 +229,7 @@
 except TypeError:
 data = first.more()
 if data:
+ self.producer_fifo.appendleft(first)
 self.producer_fifo.appendleft(data)
 continue
The asynchat test is OK when the patch is modified with the above
change.
However, then the patch does not make initiate_send() thread safe.
There is now a race condition: another thread may be allowed to run
between the two appendleft() calls, this other thread may then call
the more() method of 'first' and send the returned bytes. When that
happens, the sent data is mis-ordered.

I think we shouldn't expect asynchat to be thread safe in this regard.

What about changing:
self.producer_fifo.appendleft(first)
self.producer_fifo.appendleft(data)
To
self.producer_fifo.extendleft([data, first])
Assuming deque's extendleft is actually thread safe.

extendleft is an extension module C function (in the _collections
module) that does not release the GIL, so it is thread safe.

Great. Everybody's happy now, surely?

patch plus "self.producer_fifo.extendleft([data, first])" seems legit and I verified pyftpdlib tests pass.
Last thing missing from the patch is a test case. Pierrick can you merge test_initiate_send.py into Lib/test_asynchat.py and provide a new patch?

sorry for the delay, here is the updated patch,
shall I add a new class in Lib/test/test_asynchat.py ?

Attached are two test cases for this patch.
test_simple_producer still fails with the new patch because it should be:
 self.producer_fifo.extendleft([first, data])
instead of:
 self.producer_fifo.appendleft([data, first])
The order of the items in the list is reversed, as documented in
deque.extendleft. So the attachment also includes the corrected patch
with this single change.
I still think that if num_sent == 0, then 'first' should be put back
in the queue. This means that initiate_send should not attempt anymore
to send an empty string, which is confusing anyway, and therefore at
the beginning of initiate_send, when 'if not first', then we should
return in all cases and not only when 'first' is None.

I think you mean:
 self.producer_fifo.extendleft([data, first])
Instead of:
 self.producer_fifo.extendleft([first, data])
No?

last patch, replaced:
 self.producer_fifo.appendleft([data, first])
by:
 self.producer_fifo.extendleft([data, first])

> I think you mean:
>
> self.producer_fifo.extendleft([data, first])
>
> Instead of:
>
> self.producer_fifo.extendleft([first, data])
>
> No?
No, I do mean:
 self.producer_fifo.extendleft([first, data])
See the documentation.
Also:
>>> from collections import deque
>>> a = deque([0, 1, 2, 3])
>>> a.extendleft([11, 22])
>>> a
deque([22, 11, 0, 1, 2, 3])

After applying the last patch cpython.asyncore_3.patch (while
cpython.asyncore_2.patch does not fail):
======================================================================
FAIL: test_simple_producer (test.test_asynchat.TestAsynchat)
----------------------------------------------------------------------
Traceback (most recent call last):
 File "Lib/test/test_asynchat.py", line 198, in test_simple_producer
 self.fail("join() timed out")
AssertionError: join() timed out
======================================================================
FAIL: test_simple_producer (test.test_asynchat.TestAsynchat_WithPoll)
----------------------------------------------------------------------
Traceback (most recent call last):
 File "Lib/test/test_asynchat.py", line 198, in test_simple_producer
 self.fail("join() timed out")
AssertionError: join() timed out

Fix patch from Xavier's comment, sorry for the delay.
Lib/test/test_asynchat.py passes (Ran 27 tests in 1.431s)