Run tornado gen.coroutines in the context of a task.

[BrandonTheBuilder]

This PR re-factors the gen.coroutine runner to exercise the generator in the context of a single asyncio.Task.
With this update gen.coroutines will work more like native coroutines and work with the asyncio.current_task api. One change in functionality is that execution will always be deferred to the IOLoop when previously the generator was exercised to the first yield on instantiation.

[majorgreys]

Thank you @BrandonTheBuilder for contributing this PR!

[bdarnell]

Thanks for the contribution, and I apologize for being slow to review it. There's subtle code here and it needs a lot of attention.

This looks like it could have significant performance overhead: we now have both a Task and a Runner, we've lost the first_yielded optimization, we're creating an asyncio.Event for every yield, etc. Performance-sensitive code should be moving to native coroutines, but it's still undesirable to slow down decorated coroutines if we don't have to.

It looks to me like it might be simpler to make @coroutine support contextvars directly than to integrate it with asyncio tasks: save contextvars.copy_context() when creating the runner, and use ctx.run when we call back into the runner from add_future. Have you looked into this possibility?

[bdarnell]

I would have expected this to be simply await self.future. We might need to create an Event for some edge cases (concurrent futures, or the moment singleton), but most of the time I think awaiting the future directly would work.

[bdarnell]

Style note: Tornado uses a leading underscore on "private" class/instance attributes, but local variables should just have plain names like step.

[BrandonTheBuilder]

Thanks for the contribution, and I apologize for being slow to review it. There's subtle code here and it needs a lot of attention.

This looks like it could have significant performance overhead: we now have both a Task and a Runner, we've lost the first_yielded optimization, we're creating an asyncio.Event for every yield, etc. Performance-sensitive code should be moving to native coroutines, but it's still undesirable to slow down decorated coroutines if we don't have to.

It looks to me like it might be simpler to make @coroutine support contextvars directly than to integrate it with asyncio tasks: save contextvars.copy_context() when creating the runner, and use ctx.run when we call back into the runner from add_future. Have you looked into this possibility?

This is some awesome feedback! I totally understand taking time to fully review this there is definitely a lot going on here. It does lose the first yield optimization, I had the same thought though that performance critical code should be using native coroutines. Using contextvars directly might solve this problem as well, and not lose the performance optimization. I will test it out and try to push something up for further review soon.

[owais]

Thanks for the PR. I'm trying to patch python-tornado (https://github.com/opentracing-contrib/python-tornado) and tornado 6 coroutines actually don't work with context vars. I tried this patch and it fixes the problem. Would be great if this patch or a different solution could be merged in. Removal of the stack context feature and coroutines not working with contextvars means there is no reliable way to pass contextual information down when using coroutines.

I guess we know what the problem is here and how it manifests but I can post an demo app that shows how using contextvars doesn't work currently but works with this patch applied. LMK if that would be useful. Thanks.

[bdarnell]

@owais If you're still interested, an example demonstrating where contextvars aren't working would be helpful, thanks. We have a test showing that basic usage of contextvars works (#2561) and I'm not sure where things might be going wrong. Was your issue the same as in #2731? I'll look into this diff but something more focused would help speed the process up.

But now that I'm returning to this PR, I see I may have gotten things mixed up. The original PR and commit messages talk about supporting current_task and friends but I responded with a focus on contextvars (and improving compatibility with contextvars seems like a side benefit of the task change). @BrandonTheBuilder what's your real motivation behind this PR?

I'm not sure that supporting current_task for @gen.coroutine should be a goal. My priority for legacy interfaces like this is backwards-compatibility - we don't want users of @gen.coroutine to see changes (performance or otherwise) as an obstacle to upgrading to the latest version of Tornado. And users who want current_task have the option of phasing out @gen.coroutine from their applications (with a side benefit of improved performance).

[BrandonTheBuilder]

@bdarnell The initial motivation behind this PR was to get the current_task api to work with gen.coroutines in order to support context tracing with the newrelic-python-agent. https://github.com/newrelic/newrelic-python-agent/blob/51af0379f032737d307b03650e15945218fe94e7/newrelic/core/trace_cache.py#L127

[bdarnell]

@BrandonTheBuilder OK, so the contextvars discussion is mostly a distraction for this PR. Let's keep the discussion here focused on what it would take to support current_task, and move any discussion of fixing contextvars without supporting current_task over to #2731 (which may or may not be necessary depending on the outcome here - supporting current_task will likely entail fixing contextvars)

If the goal is to support current_task, there aren't really any shortcuts - we need to call create_task up front and give up the first-yielded optimization. We might be able to reduce the performance cost of this PR by eliminating the Event, but it might get tricky. Or maybe there's a much simpler version of this change that gets rid of the Runner? This is incomplete but I wonder if something like this could work:

def coroutine(f):
    async def wrapper(*a, **kw):
        for i in f(*a, **kw):
          await handle_yielded(i)
    return wrapper

If something like that works, I think I'd support the change. Otherwise, I'm not sure it's a good idea to try and make @gen.coroutine support current_task. It's an expensive and risky change to make to an interface that exists for backwards-compatibility, and it doesn't even work particularly well for the context tracing use case (with this change, when one coroutine calls another, they're two independent tasks, as opposed to the handling of native coroutines where only the outer one gets a task).

Also, while it wouldn't support newrelic as-is, if contextvars were fully supported by coroutines,, you could put a task id in the contextvars to give it another place to look for the current context.