[issue17925] asynchat.async_chat.initiate_send : del deque[0] is not safe

[Irit Katriel]

Change by Irit Katriel :


--
resolution:  -> wont fix
stage:  -> resolved
status: open -> closed
superseder:  -> Close asyncore/asynchat/smtpd  issues and list them here

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17925] asynchat.async_chat.initiate_send : del deque[0] is not safe

[Serhiy Storchaka]

Change by Serhiy Storchaka :


--
type: crash -> behavior

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17925] asynchat.async_chat.initiate_send : del deque[0] is not safe

[Pierrick Koch]

Pierrick Koch added the comment:

Fix patch from Xavier's comment, sorry for the delay.

Lib/test/test_asynchat.py passes (Ran 27 tests in 1.431s)

--
Added file: http://bugs.python.org/file33898/cpython.asyncore_4.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17925
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17925] asynchat.async_chat.initiate_send : del deque[0] is not safe

[Xavier de Gaye]

Xavier de Gaye added the comment:

 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])

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17925
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17925] asynchat.async_chat.initiate_send : del deque[0] is not safe

[Xavier de Gaye]

Xavier de Gaye added the comment:

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17925
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17925] asynchat.async_chat.initiate_send : del deque[0] is not safe

[Pierrick Koch]

Pierrick Koch added the comment:

last patch, replaced:

  self.producer_fifo.appendleft([data, first])

by:

  self.producer_fifo.extendleft([data, first])

--
Added file: http://bugs.python.org/file30612/cpython.asyncore_3.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17925
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17925] asynchat.async_chat.initiate_send : del deque[0] is not safe

[Xavier de Gaye]

Xavier de Gaye added the comment:

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.

--
Added file: http://bugs.python.org/file30574/cpython.asyncore_2.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17925
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17925] asynchat.async_chat.initiate_send : del deque[0] is not safe

[Andrew Stormont]

Andrew Stormont added the comment:

I think you mean:

self.producer_fifo.extendleft([data, first])

Instead of:

self.producer_fifo.extendleft([first, data])

No?

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17925
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17925] asynchat.async_chat.initiate_send : del deque[0] is not safe

[Pierrick Koch]

Pierrick Koch added the comment:

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

--
Added file: http://bugs.python.org/file30545/cpython.asyncore.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17925
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17925] asynchat.async_chat.initiate_send : del deque[0] is not safe

[Andrew Stormont]

Andrew Stormont added the comment:

Great.  Everybody's happy now, surely?

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17925
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17925] asynchat.async_chat.initiate_send : del deque[0] is not safe

[Giampaolo Rodola']

Giampaolo Rodola' added the comment:

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?

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17925
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17925] asynchat.async_chat.initiate_send : del deque[0] is not safe

[Xavier de Gaye]

Xavier de Gaye added the comment:

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17925
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17925] asynchat.async_chat.initiate_send : del deque[0] is not safe

[Giampaolo Rodola']

Giampaolo Rodola' added the comment:

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17925
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17925] asynchat.async_chat.initiate_send : del deque[0] is not safe

[Andrew Stormont]

Andrew Stormont added the comment:

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17925
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17925] asynchat.async_chat.initiate_send : del deque[0] is not safe

[Xavier de Gaye]

Xavier de Gaye added the comment:

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17925
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17925] asynchat.async_chat.initiate_send : del deque[0] is not safe

[Andrew Stormont]

Andrew Stormont added the comment:

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

--
nosy: +andy_js

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17925
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17925] asynchat.async_chat.initiate_send : del deque[0] is not safe

[Giampaolo Rodola']

Giampaolo Rodola' added the comment:

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17925
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17925] asynchat.async_chat.initiate_send : del deque[0] is not safe

[Xavier de Gaye]

Xavier de Gaye added the comment:

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().

--
nosy: +xdegaye
Added file: http://bugs.python.org/file30186/test_initiate_send.py

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17925
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17925] asynchat.async_chat.initiate_send : del deque[0] is not safe

[Pierrick Koch]

New submission from Pierrick Koch:

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

--
components: Library (Lib)
files: asynchat.async_chat.initiate_send.deldeque.patch
keywords: patch
messages: 188652
nosy: Pierrick.Koch
priority: normal
severity: normal
status: open
title: asynchat.async_chat.initiate_send : del deque[0] is not safe
type: crash
versions: Python 3.3
Added file: 
http://bugs.python.org/file30165/asynchat.async_chat.initiate_send.deldeque.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17925
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue17925] asynchat.async_chat.initiate_send : del deque[0] is not safe

[R. David Murray]

Changes by R. David Murray rdmur...@bitdance.com:


--
nosy: +giampaolo.rodola

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue17925
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com