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

bpo-32591: Add native coroutine origin tracking #5250

Merged
merged 9 commits into from
Jan 21, 2018
7 changes: 7 additions & 0 deletions Doc/library/inspect.rst
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,10 @@ attributes:
+-----------+-------------------+---------------------------+
| | cr_code | code |
+-----------+-------------------+---------------------------+
| | cr_origin | where coroutine was |
| | | created, if coroutine |
| | | origin tracking is enabled|
+-----------+-------------------+---------------------------+
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a link to the set_coroutine_origin_tracking_depth documentation snippet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, that was my original thought. The problem is that

:func:`set_coroutine_origin_tracking_depth`

is too long to fit in the ascii-art box. So... either we need a shorter name for the function, or we need to redraw this whole giant table, and I couldn't think of a satisfactory way to do either in the 2 minutes I spent thinking about it :-). Any suggestions?

I guess we could use that weird ReST substitution thing? I'm not sure how that works.

Copy link
Member

Choose a reason for hiding this comment

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

| builtin | __doc__ | documentation string |
+-----------+-------------------+---------------------------+
| | __name__ | original name of this |
Expand All @@ -234,6 +238,9 @@ attributes:
The ``__name__`` attribute of generators is now set from the function
name, instead of the code name, and it can now be modified.

.. versionchanged:: 3.7

Add ``cr_origin`` attribute to coroutines.

.. function:: getmembers(object[, predicate])

Expand Down
21 changes: 21 additions & 0 deletions Doc/library/sys.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1212,6 +1212,27 @@ always available.
This function has been added on a provisional basis (see :pep:`411`
for details.)

.. function:: set_coroutine_origin_tracking_depth(depth)

Allows enabling or disabling coroutine origin tracking. When
enabled, the ``cr_origin`` attribute on coroutine objects will
contain a list of (filename, line number, function name) tuples
describing the traceback where the coroutine object was created.
When disabled, ``cr_origin`` will be None.
Copy link
Member

Choose a reason for hiding this comment

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

Need to specify how the list is ordered.


To enable, pass a *depth* value greater than zero; this sets the
number of frames whose information will be captured. To disable,
pass set *depth* to zero.

Returns the old value of *depth*.

This setting is thread-local.
Copy link
Member

Choose a reason for hiding this comment

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

thread-local -> thread-specific


.. versionadded:: 3.7

.. note::
This function has been added on a provisional basis (see :pep:`411`
for details.) Use it only for debugging purposes.

.. function:: set_coroutine_wrapper(wrapper)

Expand Down
1 change: 1 addition & 0 deletions Include/ceval.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ PyAPI_FUNC(PyObject *) PyEval_CallMethod(PyObject *obj,
#ifndef Py_LIMITED_API
PyAPI_FUNC(void) PyEval_SetProfile(Py_tracefunc, PyObject *);
PyAPI_FUNC(void) PyEval_SetTrace(Py_tracefunc, PyObject *);
PyAPI_FUNC(int) _PyEval_SetCoroutineOriginTrackingDepth(int new_depth);
PyAPI_FUNC(void) _PyEval_SetCoroutineWrapper(PyObject *);
PyAPI_FUNC(PyObject *) _PyEval_GetCoroutineWrapper(void);
PyAPI_FUNC(void) _PyEval_SetAsyncGenFirstiter(PyObject *);
Expand Down
1 change: 1 addition & 0 deletions Include/genobject.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ PyAPI_FUNC(void) _PyGen_Finalize(PyObject *self);
#ifndef Py_LIMITED_API
typedef struct {
_PyGenObject_HEAD(cr)
PyObject *cr_origin;
} PyCoroObject;

PyAPI_DATA(PyTypeObject) PyCoro_Type;
Expand Down
2 changes: 2 additions & 0 deletions Include/pystate.h
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,8 @@ typedef struct _ts {
void (*on_delete)(void *);
void *on_delete_data;

int coroutine_origin_tracking_depth;

PyObject *coroutine_wrapper;
int in_coroutine_wrapper;

Expand Down
4 changes: 4 additions & 0 deletions Include/warnings.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ PyErr_WarnExplicitFormat(PyObject *category,
#define PyErr_Warn(category, msg) PyErr_WarnEx(category, msg, 1)
#endif

#ifndef Py_LIMITED_API
PyAPI_FUNC(void) _PyErr_WarnUnawaitedCoroutine(PyObject *coro);
Copy link
Member

@1st1 1st1 Jan 20, 2018

Choose a reason for hiding this comment

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

Strictly speaking we don't need PyAPI_FUNC here, as warnings.c|h will be linked with genobject.c and we are not going to use it in extensions.

#endif

#ifdef __cplusplus
}
#endif
Expand Down
49 changes: 16 additions & 33 deletions Lib/asyncio/base_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
from . import sslproto
from . import tasks
from .log import logger
from .constants import DEBUG_STACK_DEPTH
Copy link
Member

Choose a reason for hiding this comment

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

In asyncio we always import modules, so please change to from . import constants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like in the line above, from .log import logger? :-)

(yes I fixed it)

Copy link
Member

Choose a reason for hiding this comment

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

That logger import is probably the only offender, will fix it in a separate PR :)



__all__ = 'BaseEventLoop',
Expand Down Expand Up @@ -224,7 +225,8 @@ def __init__(self):
self.slow_callback_duration = 0.1
self._current_handle = None
self._task_factory = None
self._coroutine_wrapper_set = False
self._coroutine_origin_tracking_enabled = False
self._coroutine_origin_tracking_saved_depth = None

if hasattr(sys, 'get_asyncgen_hooks'):
# Python >= 3.6
Expand Down Expand Up @@ -382,7 +384,7 @@ def run_forever(self):
if events._get_running_loop() is not None:
raise RuntimeError(
'Cannot run the event loop while another loop is running')
self._set_coroutine_wrapper(self._debug)
self._set_coroutine_origin_tracking(self._debug)
self._thread_id = threading.get_ident()
if self._asyncgens is not None:
old_agen_hooks = sys.get_asyncgen_hooks()
Expand All @@ -398,7 +400,7 @@ def run_forever(self):
self._stopping = False
self._thread_id = None
events._set_running_loop(None)
self._set_coroutine_wrapper(False)
self._set_coroutine_origin_tracking(False)
if self._asyncgens is not None:
sys.set_asyncgen_hooks(*old_agen_hooks)

Expand Down Expand Up @@ -1531,39 +1533,20 @@ def _run_once(self):
handle._run()
handle = None # Needed to break cycles when an exception occurs.

def _set_coroutine_wrapper(self, enabled):
try:
set_wrapper = sys.set_coroutine_wrapper
get_wrapper = sys.get_coroutine_wrapper
except AttributeError:
return

enabled = bool(enabled)
if self._coroutine_wrapper_set == enabled:
def _set_coroutine_origin_tracking(self, enabled):
if enabled == self._coroutine_origin_tracking_enabled:
Copy link
Member

@1st1 1st1 Jan 20, 2018

Choose a reason for hiding this comment

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

if enabled and self._coroutine_origin_tracking_enabled:?

I don't like using == for bools.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that would be different – the no-== equivalent would be:

if ((enabled and self._coroutine_origin_tracking_enabled) or (not enabled and not self._coroutine_origin_tracking_enabled)):
   ...

I find the == version easier to read. ("If the requested state equals the current state, return without doing anything.")

Copy link
Member

Choose a reason for hiding this comment

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

Got it, then I'd cast them both to bool: if bool(enabled) == bool(self._coroutine_origin_tracking_enabled):

return

wrapper = coroutines.debug_wrapper
current_wrapper = get_wrapper()

if enabled:
if current_wrapper not in (None, wrapper):
warnings.warn(
f"loop.set_debug(True): cannot set debug coroutine "
f"wrapper; another wrapper is already set "
f"{current_wrapper!r}",
RuntimeWarning)
else:
set_wrapper(wrapper)
self._coroutine_wrapper_set = True
self._coroutine_origin_tracking_saved_depth = (
sys.set_coroutine_origin_tracking_depth(DEBUG_STACK_DEPTH)
)
else:
if current_wrapper not in (None, wrapper):
warnings.warn(
f"loop.set_debug(False): cannot unset debug coroutine "
f"wrapper; another wrapper was set {current_wrapper!r}",
RuntimeWarning)
else:
set_wrapper(None)
self._coroutine_wrapper_set = False
sys.set_coroutine_origin_tracking_depth(
self._coroutine_origin_tracking_saved_depth
)
Copy link
Member

Choose a reason for hiding this comment

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

Please move the closing paren to the prev line:

sys.set_coroutine_origin_tracking_depth(
    self._coroutine_origin_tracking_saved_depth)

(that's just the code style in asyncio package that I'd like to maintain for consistency; and I'm slowly fixing places where it's different)


self._coroutine_origin_tracking_enabled = enabled

def get_debug(self):
return self._debug
Expand All @@ -1572,4 +1555,4 @@ def set_debug(self, enabled):
self._debug = enabled

if self.is_running():
self._set_coroutine_wrapper(enabled)
self.call_soon_threadsafe(self._set_coroutine_origin_tracking, enabled)
10 changes: 0 additions & 10 deletions Lib/asyncio/coroutines.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,6 @@ def _is_debug_mode():
_DEBUG = _is_debug_mode()


def debug_wrapper(gen):
# This function is called from 'sys.set_coroutine_wrapper'.
# We only wrap here coroutines defined via 'async def' syntax.
# Generator-based coroutines are wrapped in @coroutine
# decorator.
return CoroWrapper(gen, None)


class CoroWrapper:
# Wrapper for coroutine object in _DEBUG mode.

Expand Down Expand Up @@ -141,8 +133,6 @@ def coroutine(func):
if inspect.iscoroutinefunction(func):
# In Python 3.5 that's all we need to do for coroutines
# defined with "async def".
# Wrapping in CoroWrapper will happen via
# 'sys.set_coroutine_wrapper' function.
Copy link
Member

Choose a reason for hiding this comment

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

We can also remove cr_* properties from CoroWrapper.

return func

if inspect.isgeneratorfunction(func):
Expand Down
39 changes: 12 additions & 27 deletions Lib/test/test_asyncio/test_pep492.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Tests support for new syntax introduced by PEP 492."""

import sys
import types
import unittest

Expand Down Expand Up @@ -148,35 +149,19 @@ async def foo():
data = self.loop.run_until_complete(foo())
self.assertEqual(data, 'spam')

@mock.patch('asyncio.coroutines.logger')
def test_async_def_wrapped(self, m_log):
async def foo():
pass
async def start():
foo_coro = foo()
self.assertRegex(
repr(foo_coro),
r'<CoroWrapper .*\.foo\(\) running at .*pep492.*>')

with support.check_warnings((r'.*foo.*was never',
RuntimeWarning)):
foo_coro = None
support.gc_collect()
self.assertTrue(m_log.error.called)
message = m_log.error.call_args[0][0]
self.assertRegex(message,
r'CoroWrapper.*foo.*was never')

self.loop.set_debug(True)
self.loop.run_until_complete(start())
def test_debug_mode_manages_coroutine_origin_tracking(self):
def get_depth():
depth = sys.set_coroutine_origin_tracking_depth(0)
sys.set_coroutine_origin_tracking_depth(depth)
return depth

async def start():
foo_coro = foo()
task = asyncio.ensure_future(foo_coro, loop=self.loop)
self.assertRegex(repr(task), r'Task.*foo.*running')
self.assertTrue(get_depth() > 0)

self.loop.set_debug(True)
self.loop.run_until_complete(start())

self.assertEqual(get_depth(), 0)

def test_types_coroutine(self):
def gen():
Expand Down Expand Up @@ -226,9 +211,9 @@ async def runner():
t.cancel()

self.loop.set_debug(True)
with self.assertRaisesRegex(
RuntimeError,
r'Cannot await.*test_double_await.*\bafunc\b.*while.*\bsleep\b'):
with self.assertRaises(
RuntimeError,
msg='coroutine is being awaited already'):

self.loop.run_until_complete(runner())

Expand Down
Loading