classification
Title: q.put(some_tuple) fails when PYTHONASYNCIODEBUG=1
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gvanrossum Nosy List: Arfrever, benjamin.peterson, giampaolo.rodola, gvanrossum, haypo, ncoghlan, pitrou, python-dev, richard.kiss, yselivanov
Priority: normal Keywords: patch

Created on 2014-04-13 01:10 by richard.kiss, last changed 2014-06-30 12:41 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
put_get_bug.py richard.kiss, 2014-04-13 01:10
gen_send.diff gvanrossum, 2014-04-13 20:43
gen_send_2.diff gvanrossum, 2014-04-14 01:23
ceval.patch haypo, 2014-04-14 03:00 review
corowrapper_01.patch yselivanov, 2014-04-14 18:13
corowrapper_02.patch yselivanov, 2014-04-14 21:40
corowrapper_03.patch yselivanov, 2014-04-15 14:10
Messages (30)
msg215991 - (view) Author: Richard Kiss (richard.kiss) * Date: 2014-04-13 01:10
import asyncio
import os

def t1(q):
    yield from asyncio.sleep(0.5)
    q.put_nowait((0, 1, 2, 3, 4, 5))

def t2(q):
    v = yield from q.get()
    print(v)

q = asyncio.Queue()
asyncio.get_event_loop().run_until_complete(asyncio.wait([t1(q), t2(q)]))



When PYTHONASYNCIODEBUG is set to 1, this causes a strange error:

TypeError: send() takes 2 positional arguments but 7 were given

See also https://gist.github.com/richardkiss/10564363
msg215993 - (view) Author: Richard Kiss (richard.kiss) * Date: 2014-04-13 03:39
For a reason that I don't understand, this patch to asyncio fixes the problem:


--- a/asyncio/tasks.py	Mon Mar 31 11:31:16 2014 -0700
+++ b/asyncio/tasks.py	Sat Apr 12 20:37:02 2014 -0700
@@ -49,7 +49,8 @@
     def __next__(self):
         return next(self.gen)
 
-    def send(self, value):
+    def send(self, value, *args):
         return self.gen.send(value)
 
     def throw(self, exc):


Maybe the problem really is somewhere else, but this works.
msg216035 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-04-13 20:43
I'll be darned. It appears that generator's send() method uses METH_O, which means that it really expects a single argument, but if you pass it a tuple, it assumes that you meant each item in the tuple as a separate argument. I think a more correct fix is attached -- don't add a dummy *args to the send() method, but call self.gen.send((value,)).

I'd like to fix this upstream and add some tests first; also see http://code.google.com/p/tulip/issues/detail?id=163 (which touches upon a different problem in CoroWrapper not emulating the real generator object well enough).
msg216044 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-04-14 01:23
Heh. METH_O was *also* a red herring.  But upstream (Tulip) issue 163 *was* a good clue. I now believe that the real bug is that CoroWrapper.__iter__() has "return self" rather than "return iter(self.gen)". That fix is in the 2nd attachment.
msg216057 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-04-14 02:39
The error occurs at line "v = yield from q.get()":

Traceback (most recent call last):
  File "/home/haypo/prog/python/default/Lib/asyncio/events.py", line 39, in _run
    self._callback(*self._args)
  File "/home/haypo/prog/python/default/Lib/asyncio/tasks.py", line 357, in _wakeup
    self._step(value, None)
  File "/home/haypo/prog/python/default/Lib/asyncio/tasks.py", line 309, in _step
    self.set_exception(exc)
  File "/home/haypo/prog/python/default/Lib/asyncio/tasks.py", line 301, in _step
    result = coro.send(value)
  File "put_get_bug.py", line 23, in t2
    v = yield from q.get()
TypeError: send() takes 2 positional arguments but 7 were given

Task._step() is called with value=(0, 1, 2, 3, 4, 5) (and exc is None).
msg216058 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-04-14 02:40
gen_send.diff doesn't look like a fix but a workaround.

gen_send_2.diff lacks a unit test.
msg216060 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-04-14 03:00
I don't think that the bug comes from asyncio, but it looks like a bug in the implementation of "yield from" in CPython directly! Try with ceval.patch.

ceval.c has a fast path if the object is a generator. With PYTHONASYNCIODEBUG=1, the object is a CoroWrapper, not a generator. In this case, the slow path is used:

   retval = _PyObject_CallMethodId(reciever, &PyId_send, "O", v);

This line comes from the initial commit introducing yield-from:
---
changeset:   74356:d64ac9ab4cd0
user:        Nick Coghlan <ncoghlan@gmail.com>
date:        Fri Jan 13 21:43:40 2012 +1000
files:       Doc/library/dis.rst Doc/library/exceptions.rst Doc/reference/expressions.rst Doc/reference/simple_stmts.rst Doc/whatsnew/3.
description:
Implement PEP 380 - 'yield from' (closes #11682)
---
(The exact line changed and the line was moved, but "O" format didn't change.)

Still no unit test, I'm too tired to write one, and I'm not sure that it's a bug in ceval.c.
msg216062 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-04-14 03:19
Wow. So many fixes! :-) Are you going to be at the CPython sprint tomorrow? I'll be there in the morning but my plane leaves in the afternoon.
msg216063 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-04-14 03:28
> Are you going to be at the CPython sprint tomorrow? I'll be there in the morning but my plane leaves in the afternoon.

I organize a "Port OpenStack to Python3" sprint, but I may come also
to the CPython sprint.
msg216064 - (view) Author: Roundup Robot (python-dev) Date: 2014-04-14 03:52
New changeset 05b3a23b3836 by Benjamin Peterson in branch '3.4':
fix sending tuples to custom generator objects with yield from (closes #21209)
http://hg.python.org/cpython/rev/05b3a23b3836

New changeset d1eba2645b80 by Benjamin Peterson in branch 'default':
merge 3.4 (#21209)
http://hg.python.org/cpython/rev/d1eba2645b80
msg216065 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-04-14 03:58
Hm... Can we also commit this to 3.3?
msg216085 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-04-14 14:48
This is nice (a backport 3.3 would be even nicer) but at least for the PyPI
repo version of Tulip I'd like to have work-around so people won't run into
this when they are using a slightly outdated Python version. I'll think
about which of my work-arounds is safe for that while not breaking the
intended functionality of CoroWrapper (i.e. that it prints a warning when
destructed before it has reached the end). That may require setting an
additional flag.
msg216118 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2014-04-14 17:16
3.3 is in security-fix only mode.
msg216120 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-04-14 17:20
> 3.3 is in security-fix only mode.

Yeah, but this is a core language bug. I believe some people may be stuck on 3.3 with broken 'yield from' for whatever reason, which will cause hard to find bugs in 3.3 compatible libraries (like asyncio/tulip).  I think we can lift the security-only restriction for this specific patch, no?
msg216122 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2014-04-14 17:22
On Mon, Apr 14, 2014, at 10:20, Yury Selivanov wrote:
> 
> Yury Selivanov added the comment:
> 
> > 3.3 is in security-fix only mode.
> 
> Yeah, but this is a core language bug. I believe some people may be stuck
> on 3.3 with broken 'yield from' for whatever reason, which will cause
> hard to find bugs in 3.3 compatible libraries (like asyncio/tulip).  I
> think we can lift the security-only restriction for this specific patch,
> no?

I don't really have an opinion on this nor is it my call; I'm just
regurgitating policy.
msg216124 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-04-14 17:25
I think I have to add a work-around to Tulip anyway, because I don't want to have to tell people "you must upgrade your Python 3.3 otherwise this problem can happen" (if upgrading was easy for them they would be on 3.4 :-). So I don't care much if the 3.3 backport happens.
msg216148 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-04-14 18:13
Guido: please take a look at the patch "corowrapper_01.patch".
msg216213 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-04-14 21:02
Yuri, thanks for the test, but why would the patch need a version check? Shouldn't the work-around work equally well in Python versions that don't need it? Maybe all we need is a comment explaining that it is a work-around and a hint that eventually we should change it back?
msg216225 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-04-14 21:40
Please see the corowrapper_02.patch. I've removed the version check, now it's much simpler.
msg216261 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-04-15 01:19
OK, looks good. I tried your test with my earlier workaround and the wrapper got deallocated too early, proving that my workaround was indeed wrong and your test is useful. I am still concerned theoretically that the CoroWrapper.send() signature is different from a real generator's send() method, but I think that send() to a coroutine is an internal detail anyway, so I can live with that, and I don't see another work-around.

When you commit, can you do upstgream (Tulip) first?
msg216271 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-04-15 02:13
> [...] CoroWrapper.send() signature is different from a real generator's send() method, but I think that send() to a coroutine is an internal detail anyway [...]

Yeah, and since it's used in debug mode only, I think we should be safe.

> When you commit, can you do upstgream (Tulip) first?

Sure, this patch was for tulip code.
msg216272 - (view) Author: Roundup Robot (python-dev) Date: 2014-04-15 02:29
New changeset 0c35d3616df5 by Yury Selivanov in branch '3.4':
asyncio.tasks: Fix CoroWrapper to workaround yield-from bug in CPython < 3.4.1
http://hg.python.org/cpython/rev/0c35d3616df5

New changeset 13ff8645be57 by Yury Selivanov in branch 'default':
syncio.tasks: Fix CoroWrapper to workaround yield-from bug in CPython < 3.4.1
http://hg.python.org/cpython/rev/13ff8645be57
msg216292 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-04-15 14:10
Guido, I'm feeling a bit uncomfortable with the patch I pushed. I think we should adjust the solution, to avoid having arguments to 'gen.send' packed in two nested tuples. Please take a look at the new patch (corowrapper_03.patch). It adds some amount of ugliness, but with it in place, I'd be more sure that we don't brake anything.
msg216301 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-04-15 14:55
"I think we should adjust the solution, to avoid having arguments to 'gen.send' packed in two nested tuples."

I should check, but I think that Python create a tuple for you if you don't pass directly a tuple, so it's not very different.

Anyway, it is only used for debug, so I don't think that performances matter here.
msg216302 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-04-15 15:00
I agree with Yuri and I approve of the patch.
msg216325 - (view) Author: Roundup Robot (python-dev) Date: 2014-04-15 16:02
New changeset 2729823525fe by Yury Selivanov in branch '3.4':
asyncio.tasks: Make sure CoroWrapper.send proxies one argument correctly
http://hg.python.org/cpython/rev/2729823525fe

New changeset 552ee474f3e7 by Yury Selivanov in branch 'default':
asyncio.tasks: Make sure CoroWrapper.send proxies one argument correctly
http://hg.python.org/cpython/rev/552ee474f3e7
msg216329 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2014-04-15 16:24
> I should check, but I think that Python create a tuple for you if you don't pass directly a tuple, so it's not very different.

That's what I thought, but still, better to have the code clearly expressing what it does, than relying on obscure implementation/protocol details.
msg216902 - (view) Author: Arfrever Frehtes Taifersar Arahesis (Arfrever) * Date: 2014-04-20 13:10
The added comment contains "This workaround should be removed in 3.5.0.". Since default branch now contains Python 3.5, maybe it is time to remove workaround on default branch?
msg216905 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-04-20 15:34
IMO the comment is too aggressive. I want the workaround to stay in the codebase so CPython asyncio ans Tulip asyncio (== upstream) don't diverge.
msg221955 - (view) Author: Roundup Robot (python-dev) Date: 2014-06-30 12:40
New changeset defd09a5339a by Victor Stinner in branch '3.4':
asyncio: sync with Tulip
http://hg.python.org/cpython/rev/defd09a5339a

New changeset 8dc8c93e74c9 by Victor Stinner in branch 'default':
asyncio: sync with Tulip
http://hg.python.org/cpython/rev/8dc8c93e74c9
History
Date User Action Args
2014-06-30 12:41:00python-devsetmessages: + msg221955
2014-04-20 15:34:21gvanrossumsetmessages: + msg216905
2014-04-20 13:10:01Arfreversetnosy: + Arfrever
messages: + msg216902
2014-04-15 16:24:46yselivanovsetmessages: + msg216329
2014-04-15 16:02:26python-devsetmessages: + msg216325
2014-04-15 15:00:49gvanrossumsetmessages: + msg216302
2014-04-15 14:55:53hayposetmessages: + msg216301
2014-04-15 14:10:54yselivanovsetfiles: + corowrapper_03.patch

messages: + msg216292
2014-04-15 02:30:19yselivanovsetstatus: open -> closed
resolution: fixed
2014-04-15 02:29:46python-devsetmessages: + msg216272
2014-04-15 02:13:13yselivanovsetmessages: + msg216271
2014-04-15 01:19:26gvanrossumsetmessages: + msg216261
2014-04-14 21:40:15yselivanovsetfiles: + corowrapper_02.patch

messages: + msg216225
2014-04-14 21:02:37gvanrossumsetmessages: + msg216213
2014-04-14 18:27:16yselivanovsetstatus: closed -> open
resolution: fixed -> (no value)
2014-04-14 18:13:01yselivanovsetfiles: + corowrapper_01.patch

messages: + msg216148
2014-04-14 17:25:35gvanrossumsetmessages: + msg216124
2014-04-14 17:22:50benjamin.petersonsetmessages: + msg216122
2014-04-14 17:20:04yselivanovsetmessages: + msg216120
2014-04-14 17:16:36benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg216118
2014-04-14 14:48:21gvanrossumsetmessages: + msg216085
2014-04-14 03:58:01yselivanovsetmessages: + msg216065
2014-04-14 03:52:53python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg216064

resolution: fixed
stage: resolved
2014-04-14 03:28:21hayposetmessages: + msg216063
2014-04-14 03:19:11gvanrossumsetmessages: + msg216062
2014-04-14 03:00:25hayposetfiles: + ceval.patch
nosy: + ncoghlan
messages: + msg216060

2014-04-14 02:40:22hayposetmessages: + msg216058
2014-04-14 02:39:41hayposetmessages: + msg216057
2014-04-14 01:23:03gvanrossumsetfiles: + gen_send_2.diff

messages: + msg216044
2014-04-13 20:43:35gvanrossumsetfiles: + gen_send.diff
assignee: gvanrossum
messages: + msg216035

keywords: + patch
2014-04-13 15:25:19ned.deilysetnosy: + gvanrossum, pitrou, haypo, giampaolo.rodola, yselivanov
2014-04-13 03:39:43richard.kisssetmessages: + msg215993
2014-04-13 01:10:41richard.kisscreate