Opened 7 years ago

Last modified 7 years ago

#7955 defect new

Let inlineCallbacks be used on Cython generators

Reported by: Sirenfal Owned by: Sirenfal
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch:
Author: Sirenfal

Description

In the unwindGenerator section of the @inlineCallbacks decorator there is an explicit check to test whether the wrapped object is a generator:

        if not isinstance(gen, types.GeneratorType):
            raise TypeError(
                "inlineCallbacks requires %r to produce a generator; "
                "instead got %r" % (f, gen))

Unfortunately, Cython does not return Python's generator type (which makes sense given the nature of Cython) so this check fails. However, if you remove the check Cython code decorated with inlineCallbacks works perfectly.

It seems to be this should be duck typed. There is a related asyncio issue for Python 3.5, and that seems to also be the consensus there: https://bugs.python.org/issue24004

It seems like the old check can be replaced with this:

        if not gen:
            raise TypeError(
                "inlineCallbacks requires %r to produce a generator; instead"
                "got %r" % (f, gen))

Which produces:

TypeError: inlineCallbacks requires <built-in function host> to produce a generator; instead got None

This will allow a Cython or Python generator to function, but it won't allow you to decorate a function that doesn't yield. I'm not sure what other cases might have made the old check fail.

Thanks.

Attachments (2)

inlinecallbacks-7955-1.patch (828 bytes) - added by Sirenfal 7 years ago.
Patch for duck type test instead of generator test
inlinecallbacks-7955-2.patch (3.0 KB) - added by Sirenfal 7 years ago.

Download all attachments as: .zip

Change History (11)

Changed 7 years ago by Sirenfal

Patch for duck type test instead of generator test

comment:1 Changed 7 years ago by Sirenfal

Keywords: review added

comment:2 Changed 7 years ago by Julian Berman

Keywords: review removed

Hi! Thanks so much for the patch.

Those two conditionals are quite different. The intent of the check there is to make sure that inlineCallbacks will be able to call generator methods on the return value, and if not, to error quickly. if not gen is going to basically only fail when the return value of the function is falsy, which isn't the intent of the check. A function that returns [1, 2, 3] should still raise an exception with the current behavior. You'll notice that after your changeset, twisted.test.test_defgen.InlineCallbacksTests.test_nonGeneratorReturn fails, which is a test specifically checking this behavior.

I'm not sure what the correct "fix" is for this new behavior -- I skimmed the upstream ticket and it certainly seems like there should be an ABC that Cython generators should be registering themselves with. In the absence of that it'd seem that a new IGenerator interface could serve (having the methods that inlineCallbacks wants, which are the current set of generator methods), and then both language level generators and Cython generators could register itself with that interface. I'm sure other suggestions would be welcome if they maintain compatibility here.

Also, all changes must include tests :), so regardless of the correct solution, there's a missing test that would need to be added.

Thanks again for the submission regardless.

comment:3 Changed 7 years ago by Glyph

Owner: set to Sirenfal

comment:4 Changed 7 years ago by Sirenfal

Perhaps the following instead?

if not hasattr(gen, 'next'):

This passes the test you mentioned, as well as all other tests. While it's not the ideal check (Unless I'm misunderstanding something ABC seems to only be available in Py3), it seems to maintain compatibility and allows Cython functions to work under Python 2.7.

EDIT: To be clear, I mean the standard library generator ABC only exists in Python 3.5? I'm not very familiar with the ABC module though, I might be missing it.

I'm aware I'm supposed to submit tests, but I'm not terribly clear on what I should be testing that isn't already tested. Adding a test for Cython decorators would seem to add an unnecessary dependency for other developers? Either way I figured I could sort that out once I have a working patch :)

Thank you for your help.

Last edited 7 years ago by Sirenfal (previous) (diff)

comment:5 Changed 7 years ago by Glyph

Keywords: defer removed

You can add an optional dependency on Cython. Or, you could add a decorator which wrapped a normal Python generator in a way which made it functionally similar to a Cython generator, and register the decorator's type in the same way that Cython ought to register its, if we come up with an external-registration API.

On Twisted, we prefer to do test-driven-development, rather than sorting things out once the patch is already ready; writing the tests first ensures that there is a test for every relevant aspect of the behavior and no unnecessary untested aspects are added.

Thank you for proceeding with this. It would be great to have inlineCallbacks work with Cython. (I am removing the "defer" keyword because we use keywords to track workflow, and "defer" Is not a workflow state.)

Changed 7 years ago by Sirenfal

comment:6 Changed 7 years ago by Sirenfal

I've created a test (patch attached) with a Cython generator. If you have Cython installed you will also have the "pyximport" module which will automatically compile a .pyx file to a Python C extension and import it. If that module is missing it will simply skip the test.

The patch also includes the aforementioned "hasattr" conditional, which duplicates the behavior of the original Twisted test while allowing duck typed generators like Cython's through.

comment:7 Changed 7 years ago by Glyph

You should probably re-attach the "review" keyword so this gets picked up by people looking for stuff to review :).

comment:8 Changed 7 years ago by Sirenfal

Keywords: review added

comment:9 Changed 7 years ago by Sirenfal

Keywords: review removed

It seems like in more complex situations some pretty strange errors can occur with this (SystemError: error return without exception set, etc). I'm going to have to investigate it on the Cython side. I'll bump this ticket if/when they help me figure out what's going on.

Note: See TracTickets for help on using tickets.