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
Prev Previous commit
Next Next commit
Address most of Yury's comments
  • Loading branch information
njsmith committed Jan 21, 2018
commit 2157af3032fe196f1088b37248641e7b09e6686b
7 changes: 5 additions & 2 deletions Doc/library/inspect.rst
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ provided as convenient choices for the second argument to :func:`getmembers`.
They also help you determine when you can expect to find the following special
attributes:

.. this function name is too big to fit in the ascii-art table below
.. |coroutine-origin-link| replace:: :func:`sys.set_coroutine_origin_tracking_depth`

+-----------+-------------------+---------------------------+
| Type | Attribute | Description |
+===========+===================+===========================+
Expand Down Expand Up @@ -216,8 +219,8 @@ attributes:
| | cr_code | code |
+-----------+-------------------+---------------------------+
| | cr_origin | where coroutine was |
| | | created, if coroutine |
| | | origin tracking is enabled|
| | | created, or ``None``. See |
| | | |coroutine-origin-link| |
+-----------+-------------------+---------------------------+
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 |
+-----------+-------------------+---------------------------+
Expand Down
15 changes: 12 additions & 3 deletions Doc/library/sys.rst
Original file line number Diff line number Diff line change
Expand Up @@ -686,6 +686,10 @@ always available.
This function has been added on a provisional basis (see :pep:`411`
for details.) Use it only for debugging purposes.

.. deprecated:: 3.7
The coroutine wrapper functionality has been deprecated, and
will be removed in 3.8. See :issue:`32591` for details.


.. data:: hash_info

Expand Down Expand Up @@ -1217,16 +1221,17 @@ always available.
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.
describing the traceback where the coroutine object was created,
with the most recent call first. When disabled, ``cr_origin`` will
be None.

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.
This setting is thread-specific.

.. versionadded:: 3.7

Expand Down Expand Up @@ -1273,6 +1278,10 @@ always available.
This function has been added on a provisional basis (see :pep:`411`
for details.) Use it only for debugging purposes.

.. deprecated:: 3.7
The coroutine wrapper functionality has been deprecated, and
will be removed in 3.8. See :issue:`32591` for details.

.. function:: _enablelegacywindowsfsencoding()

Changes the default filesystem encoding and errors mode to 'mbcs' and
Expand Down
3 changes: 3 additions & 0 deletions Doc/whatsnew/3.7.rst
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,9 @@ sys

Added :attr:`sys.flags.dev_mode` flag for the new development mode.

Deprecated :func:`sys.set_coroutine_wrapper` and
:func:`sys.get_coroutine_wrapper`.

time
----

Expand Down
2 changes: 1 addition & 1 deletion Include/warnings.h
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ PyErr_WarnExplicitFormat(PyObject *category,
#endif

#ifndef Py_LIMITED_API
PyAPI_FUNC(void) _PyErr_WarnUnawaitedCoroutine(PyObject *coro);
void _PyErr_WarnUnawaitedCoroutine(PyObject *coro);
#endif

#ifdef __cplusplus
Expand Down
11 changes: 5 additions & 6 deletions Lib/asyncio/base_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,13 @@
except ImportError: # pragma: no cover
ssl = None

from . import constants
from . import coroutines
from . import events
from . import futures
from . import sslproto
from . import tasks
from .log import logger
from .constants import DEBUG_STACK_DEPTH


__all__ = 'BaseEventLoop',
Expand Down Expand Up @@ -1534,17 +1534,16 @@ def _run_once(self):
handle = None # Needed to break cycles when an exception occurs.

def _set_coroutine_origin_tracking(self, enabled):
if enabled == self._coroutine_origin_tracking_enabled:
if bool(enabled) == bool(self._coroutine_origin_tracking_enabled):
return

if enabled:
self._coroutine_origin_tracking_saved_depth = (
sys.set_coroutine_origin_tracking_depth(DEBUG_STACK_DEPTH)
)
sys.set_coroutine_origin_tracking_depth(
constants.DEBUG_STACK_DEPTH))
else:
sys.set_coroutine_origin_tracking_depth(
self._coroutine_origin_tracking_saved_depth
)
self._coroutine_origin_tracking_saved_depth)

self._coroutine_origin_tracking_enabled = enabled

Expand Down
23 changes: 0 additions & 23 deletions Lib/asyncio/coroutines.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,39 +79,16 @@ def gi_code(self):
return self.gen.gi_code

def __await__(self):
cr_await = getattr(self.gen, 'cr_await', None)
if cr_await is not None:
raise RuntimeError(
f"Cannot await on coroutine {self.gen!r} while it's "
f"awaiting for {cr_await!r}")
return self

@property
def gi_yieldfrom(self):
return self.gen.gi_yieldfrom

@property
def cr_await(self):
return self.gen.cr_await

@property
def cr_running(self):
return self.gen.cr_running

@property
def cr_code(self):
return self.gen.cr_code

@property
def cr_frame(self):
return self.gen.cr_frame

def __del__(self):
# Be careful accessing self.gen.frame -- self.gen might not exist.
gen = getattr(self, 'gen', None)
frame = getattr(gen, 'gi_frame', None)
if frame is None:
frame = getattr(gen, 'cr_frame', None)
if frame is not None and frame.f_lasti == -1:
msg = f'{self!r} was never yielded from'
tb = getattr(self, '_source_traceback', ())
Expand Down
20 changes: 11 additions & 9 deletions Lib/test/test_coroutines.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
from contextlib import contextmanager, closing
import contextlib
import copy
import inspect
import pickle
import re
import sys
import types
import unittest
import warnings
import re
from test import support


Expand Down Expand Up @@ -59,7 +59,7 @@ def run_async__await__(coro):
return buffer, result


@contextmanager
@contextlib.contextmanager
def silence_coro_gc():
with warnings.catch_warnings():
warnings.simplefilter("ignore")
Expand Down Expand Up @@ -1975,9 +1975,11 @@ def wrap(gen):
wrapped = gen
return gen

self.assertIsNone(sys.get_coroutine_wrapper())
with self.assertWarns(DeprecationWarning):
self.assertIsNone(sys.get_coroutine_wrapper())

sys.set_coroutine_wrapper(wrap)
with self.assertWarns(DeprecationWarning):
sys.set_coroutine_wrapper(wrap)
self.assertIs(sys.get_coroutine_wrapper(), wrap)
try:
f = foo()
Expand Down Expand Up @@ -2053,13 +2055,13 @@ def test_origin_tracking(self):
async def corofn():
pass

with closing(corofn()) as coro:
with contextlib.closing(corofn()) as coro:
self.assertIsNone(coro.cr_origin)

self.assertEqual(sys.set_coroutine_origin_tracking_depth(1), 0)

fname, lineno = self.here()
with closing(corofn()) as coro:
with contextlib.closing(corofn()) as coro:
self.assertEqual(coro.cr_origin,
[(fname, lineno + 1, "test_origin_tracking")])

Expand All @@ -2068,14 +2070,14 @@ def nested():
return (self.here(), corofn())
fname, lineno = self.here()
((nested_fname, nested_lineno), coro) = nested()
with closing(coro):
with contextlib.closing(coro):
self.assertEqual(coro.cr_origin,
[(nested_fname, nested_lineno, "nested"),
(fname, lineno + 1, "test_origin_tracking")])

# Check we handle running out of frames correctly
sys.set_coroutine_origin_tracking_depth(1000)
with closing(corofn()) as coro:
with contextlib.closing(corofn()) as coro:
self.assertTrue(2 < len(coro.cr_origin) < 1000)

# We can't set depth negative
Expand Down
6 changes: 6 additions & 0 deletions Lib/warnings.py
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,12 @@ def extract():
msg_lines.append("Coroutine created at (most recent call last)\n")
msg_lines += traceback.format_list(list(extract()))
msg = "".join(msg_lines).rstrip("\n")
# Passing source= here means that if the user happens to have tracemalloc
# enabled and tracking where the coroutine was created, the warning will
# contain that traceback. This does mean that if they have *both*
# coroutine origin tracking *and* tracemalloc enabled, they'll get two
# partially-redundant tracebacks. If we wanted to be clever we could
# probably detect this case and avoid it, but for now we don't bother.
warn(msg, category=RuntimeWarning, stacklevel=2, source=coro)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Small, easy to miss point here: passing source=coro here means that if the user happens to have tracemalloc enabled, its origin traceback will be included in the warning. Kinda neat! It does mean if you have both set_coroutine_origin_tracking_depth and tracemalloc enabled you'll get two at-least-partially-redundant tracebacks in the same warning. I don't know that I care that much about this but FYI.

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.

I think a redundant warning is OK. tracemalloc is enabled quite rarely. I also think what you wrote in this comment should be reflected in a code comment in case someone debugs the double warnings issue :)



Expand Down
74 changes: 43 additions & 31 deletions Objects/genobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,9 @@ gen_traverse(PyGenObject *gen, visitproc visit, void *arg)
Py_VISIT(gen->gi_code);
Py_VISIT(gen->gi_name);
Py_VISIT(gen->gi_qualname);
if (((PyCodeObject *)gen->gi_code)->co_flags & CO_COROUTINE)
if (((PyCodeObject *)gen->gi_code)->co_flags & CO_COROUTINE) {
Py_VISIT(((PyCoroObject *)gen)->cr_origin);
Copy link
Member

Choose a reason for hiding this comment

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

If you only store ints and strings in tuples of that list (without cycles), you can skip the Py_VISIT call.

}
return exc_state_traverse(&gen->gi_exc_state, visit, arg);
}

Expand Down Expand Up @@ -76,8 +77,9 @@ _PyGen_Finalize(PyObject *self)
if (gen->gi_code != NULL &&
((PyCodeObject *)gen->gi_code)->co_flags & CO_COROUTINE &&
gen->gi_frame->f_lasti == -1) {
if (!error_value)
if (!error_value) {
_PyErr_WarnUnawaitedCoroutine((PyObject *)gen);
}
}
else {
res = gen_close(gen, NULL);
Expand Down Expand Up @@ -136,8 +138,9 @@ gen_dealloc(PyGenObject *gen)
gen->gi_frame->f_gen = NULL;
Py_CLEAR(gen->gi_frame);
}
if (((PyCodeObject *)gen->gi_code)->co_flags & CO_COROUTINE)
if (((PyCodeObject *)gen->gi_code)->co_flags & CO_COROUTINE) {
Py_CLEAR(((PyCoroObject *)gen)->cr_origin);
}
Py_CLEAR(gen->gi_code);
Py_CLEAR(gen->gi_name);
Py_CLEAR(gen->gi_qualname);
Expand Down Expand Up @@ -1160,47 +1163,56 @@ PyTypeObject _PyCoroWrapper_Type = {
0, /* tp_free */
};

static PyObject *
compute_cr_origin(int origin_depth)
{
PyObject *cr_origin = PyList_New(origin_depth);
PyFrameObject *frame = PyEval_GetFrame();
int i = 0;
for (; frame && i < origin_depth; ++i) {
PyObject *frameinfo = Py_BuildValue(
"OiO",
frame->f_code->co_filename,
PyFrame_GetLineNumber(frame),
frame->f_code->co_name);
if (!frameinfo) {
Py_DECREF(cr_origin);
return NULL;
}
PyList_SET_ITEM(cr_origin, i, frameinfo);
frame = frame->f_back;
}
/* Truncate the list if necessary */
if (i < origin_depth) {
if (PyList_SetSlice(cr_origin, i, origin_depth, NULL) < 0) {
Py_DECREF(cr_origin);
return NULL;
}
}

return cr_origin;
}

PyObject *
PyCoro_New(PyFrameObject *f, PyObject *name, PyObject *qualname)
{
PyObject *coro = gen_new_with_qualname(&PyCoro_Type, f, name, qualname);
if (!coro)
if (!coro) {
return NULL;
}

PyThreadState *tstate = PyThreadState_GET();
int depth = tstate->coroutine_origin_tracking_depth;
int origin_depth = tstate->coroutine_origin_tracking_depth;

if (depth == 0) {
if (origin_depth == 0) {
((PyCoroObject *)coro)->cr_origin = NULL;
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Again, per PEP 7:

if ( ... ) {
}
else {
}

Copy link
Member

Choose a reason for hiding this comment

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

Also I'd like this whole else branch to be in a separate helper function.

PyObject *origin = PyList_New(depth);
/* Immediately pass ownership to coro, so on error paths we don't have
to worry about it separately. */
((PyCoroObject *)coro)->cr_origin = origin;
PyFrameObject *frame = PyEval_GetFrame();
int i = 0;
for (; i < depth; ++i) {
if (!frame)
break;

PyObject *frameinfo = Py_BuildValue(
"OiO",
frame->f_code->co_filename,
PyFrame_GetLineNumber(frame),
frame->f_code->co_name);
if (!frameinfo) {
Py_DECREF(coro);
return NULL;
}
PyList_SET_ITEM(origin, i, frameinfo);

frame = frame->f_back;
}
/* Truncate the list if necessary */
if (PyList_SetSlice(origin, i, depth, NULL) < 0) {
PyObject *cr_origin = compute_cr_origin(origin_depth);
if (!cr_origin) {
Py_DECREF(coro);
return NULL;
}
((PyCoroObject *)coro)->cr_origin = cr_origin;
}

return coro;
Expand Down
Loading