| 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) *  |
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) * |
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 (vstinner) * |
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 (vstinner) * |
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 (vstinner) * |
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) * |
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 (vstinner) * |
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) * |
Date: 2014-04-14 03:58 |
| Hm... Can we also commit this to 3.3? |
|
|
| msg216085 - (view) |
Author: Guido van Rossum (gvanrossum) * |
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) * |
Date: 2014-04-14 17:16 |
| 3.3 is in security-fix only mode. |
|
|
| msg216120 - (view) |
Author: Yury Selivanov (yselivanov) * |
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) * |
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) * |
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) * |
Date: 2014-04-14 18:13 |
| Guido: please take a look at the patch "corowrapper_01.patch". |
|
|
| msg216213 - (view) |
Author: Guido van Rossum (gvanrossum) * |
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) * |
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) * |
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) * |
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) * |
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 (vstinner) * |
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) * |
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) * |
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) * |
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 |
|
|