q.put(some_tuple) fails when PYTHONASYNCIODEBUG=1

@gvanrossum

BPO	21209
Nosy	@gvanrossum, @ncoghlan, @pitrou, @vstinner, @giampaolo, @benjaminp, @1st1
Files	put_get_bug.py gen_send.diff gen_send_2.diff ceval.patch corowrapper_01.patch corowrapper_02.patch corowrapper_03.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/gvanrossum'
closed_at = <Date 2014-04-15.02:30:19.463>
created_at = <Date 2014-04-13.01:10:41.896>
labels = ['type-bug', 'library']
title = 'q.put(some_tuple) fails when PYTHONASYNCIODEBUG=1'
updated_at = <Date 2014-06-30.12:41:00.013>
user = 'https://bugs.python.org/richardkiss'

bugs.python.org fields:

activity = <Date 2014-06-30.12:41:00.013>
actor = 'python-dev'
assignee = 'gvanrossum'
closed = True
closed_date = <Date 2014-04-15.02:30:19.463>
closer = 'yselivanov'
components = ['Library (Lib)']
creation = <Date 2014-04-13.01:10:41.896>
creator = 'richard.kiss'
dependencies = []
files = ['34795', '34805', '34809', '34813', '34832', '34850', '34870']
hgrepos = []
issue_num = 21209
keywords = ['patch']
message_count = 30.0
messages = ['215991', '215993', '216035', '216044', '216057', '216058', '216060', '216062', '216063', '216064', '216065', '216085', '216118', '216120', '216122', '216124', '216148', '216213', '216225', '216261', '216271', '216272', '216292', '216301', '216302', '216325', '216329', '216902', '216905', '221955']
nosy_count = 10.0
nosy_names = ['gvanrossum', 'ncoghlan', 'pitrou', 'vstinner', 'giampaolo.rodola', 'benjamin.peterson', 'Arfrever', 'python-dev', 'yselivanov', 'richard.kiss']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue21209'
versions = ['Python 3.4']

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

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.

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

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.

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

gen_send.diff doesn't look like a fix but a workaround.

gen_send_2.diff lacks a unit test.

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 bpo-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.

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.

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.

New changeset 05b3a23b3836 by Benjamin Peterson in branch '3.4':
fix sending tuples to custom generator objects with yield from (closes bpo-21209)
http://hg.python.org/cpython/rev/05b3a23b3836

New changeset d1eba2645b80 by Benjamin Peterson in branch 'default':
merge 3.4 (bpo-21209)
http://hg.python.org/cpython/rev/d1eba2645b80

Hm... Can we also commit this to 3.3?

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?

Please see the corowrapper_02.patch. I've removed the version check, now it's much simpler.

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?

[...] 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.

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

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.

"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.

I agree with Yuri and I approve of the patch.

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

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.

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?

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.

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

Navigation Menu

Repository navigation

q.put(some_tuple) fails when PYTHONASYNCIODEBUG=1 #65408

7 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

q.put(some_tuple) fails when PYTHONASYNCIODEBUG=1 #65408

Description

Activity

RichardKiss commented on Apr 13, 2014

RichardKiss commented on Apr 13, 2014

gvanrossum commented on Apr 13, 2014

gvanrossum commented on Apr 14, 2014

vstinner commented on Apr 14, 2014

vstinner commented on Apr 14, 2014

vstinner commented on Apr 14, 2014

gvanrossum commented on Apr 14, 2014

vstinner commented on Apr 14, 2014

python-dev commented on Apr 14, 2014

1st1 commented on Apr 14, 2014

7 remaining items

gvanrossum commented on Apr 14, 2014

1st1 commented on Apr 14, 2014

gvanrossum commented on Apr 15, 2014

1st1 commented on Apr 15, 2014

python-dev commented on Apr 15, 2014

1st1 commented on Apr 15, 2014

vstinner commented on Apr 15, 2014

gvanrossum commented on Apr 15, 2014

python-dev commented on Apr 15, 2014

1st1 commented on Apr 15, 2014

Arfrever commented on Apr 20, 2014

gvanrossum commented on Apr 20, 2014

python-dev commented on Jun 30, 2014

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions