-
-
Notifications
You must be signed in to change notification settings - Fork 29.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
q.put(some_tuple) fails when PYTHONASYNCIODEBUG=1 #65408
Comments
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 |
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: 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. |
I organize a "Port OpenStack to Python3" sprint, but I may come also |
New changeset 05b3a23b3836 by Benjamin Peterson in branch '3.4': New changeset d1eba2645b80 by Benjamin Peterson in branch 'default': |
Hm... Can we also commit this to 3.3? |
This is nice (a backport 3.3 would be even nicer) but at least for the PyPI |
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? |
On Mon, Apr 14, 2014, at 10:20, Yury Selivanov wrote:
I don't really have an opinion on this nor is it my call; I'm just |
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. |
Guido: please take a look at the patch "corowrapper_01.patch". |
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? |
Yeah, and since it's used in debug mode only, I think we should be safe.
Sure, this patch was for tulip code. |
New changeset 0c35d3616df5 by Yury Selivanov in branch '3.4': New changeset 13ff8645be57 by Yury Selivanov in branch 'default': |
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': New changeset 552ee474f3e7 by Yury Selivanov in branch 'default': |
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': New changeset 8dc8c93e74c9 by Victor Stinner in branch 'default': |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: