Skip to content

bpo-32311: Implement asyncio.create_task() shortcut#4848

Merged
asvetlov merged 16 commits intopython:masterfrom
asvetlov:asyncio-toplevel-helpers
Dec 15, 2017
Merged

bpo-32311: Implement asyncio.create_task() shortcut#4848
asvetlov merged 16 commits intopython:masterfrom
asvetlov:asyncio-toplevel-helpers

Conversation

@asvetlov
Copy link
Copy Markdown
Contributor

@asvetlov asvetlov commented Dec 13, 2017

@asvetlov asvetlov force-pushed the asyncio-toplevel-helpers branch from 3876d5a to 67c3520 Compare December 13, 2017 19:57
@asvetlov
Copy link
Copy Markdown
Contributor Author

Working on tests and docs

@asvetlov asvetlov changed the title bpo-32309: Implement asyncio.create_task() and asyncio.run_in_executor shortcuts bpo-32311: Implement asyncio.create_task() shortcut Dec 13, 2017
Comment thread Lib/asyncio/events.py Outdated
Return a Task object.
"""
loop = get_running_loop()
return loop.create_task(coro)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this function should be defined in tasks.py.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

Comment thread Lib/asyncio/events.py Outdated
Return a Task object.
"""
loop = get_running_loop()
return loop.create_task(coro)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if you pass a Task instance to this function? I'd say we check if coro is a coroutine here and raise an error if it's not.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't loop.create_task() perform the check?
Actually python implementation has it but C has not.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, Python version has only an assert statement... Let's have an always working check in tasks.py/Task and _asynciomodule.c/Task. It will add a bit of overhead, but will improve the usability.

_asynciomodule.c/Task can have a fast path using PyCoro_CheckExact.

@1st1
Copy link
Copy Markdown
Member

1st1 commented Dec 13, 2017

BTW, do you need my help with #4799?

@asvetlov
Copy link
Copy Markdown
Contributor Author

I stuck on #4799 (comment)

Comment thread Lib/asyncio/__init__.py Outdated
__all__ = (base_events.__all__ +
coroutines.__all__ +
events.__all__ +
format_helpers.__all__ +
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've no idea why we export asyncio.extract_stack. Let's stop doing that. Also, why renaming file as part of this PR?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake -- extract_stack was implemented in events.py but not exported.

utils.py was not exist, functions was defined in events.py.
But it brings circular import dependency when I've added iscoroutine() check to C Task:

  • iscoroutine() implemented in coroutines.py
  • coroutines.py imports events.py
  • events.py imports _asyncio for get_running_loop() C optimization
  • _asyncio imports coroutines.py

That's why I've extracted stack/traceback formatting functions into utils.py but after that renamed utils.py to format_helpers.py.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, makes sense. Thanks!

self.assertEqual(1, ret)

self.loop.run_until_complete(coro())

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need a test that would test the "%R is not a coroutine" error.

Comment thread Modules/_asynciomodule.c Outdated
}
if (tmp) {
self->task_log_destroy_pending = 0;
PyErr_Format(PyExc_TypeError, "%R is not a coroutine", coro, NULL);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change the error message to "a coroutine was expected, got %R" in both C/Python implementations?

Comment thread Doc/library/asyncio-task.rst Outdated
.. function:: create_task(coro)

Wrap a :ref:`coroutine <coroutine>` *coro* into a task and schedule
its execution.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrap a :ref:`coroutine <coroutine>` *coro* into a task and schedule
its execution.  Return the task object.

Comment thread Lib/asyncio/constants.py Outdated
# Number of stack entries to capture in debug mode.
# The larger the number, the slower the operation in debug mode
# (see extract_stack() in events.py).
# (see extract_stack() in utils.py).
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utils -> format_helpers?

return func_repr


def extract_stack(f=None, limit=None):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep extract_stack in events.py?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. extract_stack is used in coroutines.py which in turn is used by _asynciomodule.c.

@asvetlov asvetlov merged commit f74ef45 into python:master Dec 15, 2017
@asvetlov asvetlov deleted the asyncio-toplevel-helpers branch December 15, 2017 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants