Skip to content
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

Closed
RichardKiss mannequin opened this issue Apr 13, 2014 · 30 comments
Closed

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

RichardKiss mannequin opened this issue Apr 13, 2014 · 30 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@RichardKiss
Copy link
Mannequin

RichardKiss mannequin commented Apr 13, 2014

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

    @RichardKiss
    Copy link
    Mannequin Author

    RichardKiss mannequin commented Apr 13, 2014

    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

    @RichardKiss RichardKiss mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Apr 13, 2014
    @RichardKiss
    Copy link
    Mannequin Author

    RichardKiss mannequin commented Apr 13, 2014

    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.

    @gvanrossum
    Copy link
    Member

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

    @gvanrossum gvanrossum self-assigned this Apr 13, 2014
    @gvanrossum
    Copy link
    Member

    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.

    @vstinner
    Copy link
    Member

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

    @vstinner
    Copy link
    Member

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

    gen_send_2.diff lacks a unit test.

    @vstinner
    Copy link
    Member

    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.

    @gvanrossum
    Copy link
    Member

    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.

    @vstinner
    Copy link
    Member

    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.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 14, 2014

    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

    @python-dev python-dev mannequin closed this as completed Apr 14, 2014
    @1st1
    Copy link
    Member

    1st1 commented Apr 14, 2014

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

    @gvanrossum
    Copy link
    Member

    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.

    @benjaminp
    Copy link
    Contributor

    3.3 is in security-fix only mode.

    @1st1
    Copy link
    Member

    1st1 commented Apr 14, 2014

    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?

    @benjaminp
    Copy link
    Contributor

    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.

    @gvanrossum
    Copy link
    Member

    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.

    @1st1
    Copy link
    Member

    1st1 commented Apr 14, 2014

    Guido: please take a look at the patch "corowrapper_01.patch".

    @1st1 1st1 reopened this Apr 14, 2014
    @gvanrossum
    Copy link
    Member

    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?

    @1st1
    Copy link
    Member

    1st1 commented Apr 14, 2014

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

    @gvanrossum
    Copy link
    Member

    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?

    @1st1
    Copy link
    Member

    1st1 commented Apr 15, 2014

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

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 15, 2014

    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

    @1st1 1st1 closed this as completed Apr 15, 2014
    @1st1
    Copy link
    Member

    1st1 commented Apr 15, 2014

    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.

    @vstinner
    Copy link
    Member

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

    @gvanrossum
    Copy link
    Member

    I agree with Yuri and I approve of the patch.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 15, 2014

    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

    @1st1
    Copy link
    Member

    1st1 commented Apr 15, 2014

    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.

    @Arfrever
    Copy link
    Mannequin

    Arfrever mannequin commented Apr 20, 2014

    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?

    @gvanrossum
    Copy link
    Member

    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.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 30, 2014

    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

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants