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

unraisablehook enhancements #12958

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
85594a7
unraisablehook enhancements
graingert Nov 13, 2024
b79f652
only raise errors if there are any
graingert Nov 13, 2024
194f9ec
cause the traceback to only show up once
graingert Nov 13, 2024
c78f84b
include tracemalloc message in unraisablehook failures
graingert Nov 13, 2024
7f30217
fix tests
graingert Nov 13, 2024
d2045ce
avoid keeping unraisable.object alive while waiting for collection
graingert Nov 13, 2024
7e7c965
use the formatted traceback if there's no exc_value
graingert Nov 13, 2024
b0f803d
Merge branch 'main' into set-unraisable-hook-per-session
graingert Nov 19, 2024
d540ada
handle failures in unraisable plugin hook
graingert Nov 19, 2024
a285cd5
refactor tracemalloc message
graingert Nov 20, 2024
f853b2c
test multiple errors
graingert Nov 20, 2024
34b3683
hide from the branch coverage check
graingert Nov 20, 2024
04705b4
test failure in unraisable processing
graingert Nov 20, 2024
4f8aeef
test that errors due to cyclic grabage are collected eventually
graingert Nov 20, 2024
90aa800
handle errors on <3.10
graingert Nov 20, 2024
055e673
use trylast=True instead of a wrapper
graingert Nov 20, 2024
3d6a023
Update testing/test_unraisableexception.py
graingert Nov 20, 2024
02ada4b
Update src/_pytest/unraisableexception.py
graingert Nov 20, 2024
84e7311
add newsfragment
graingert Nov 21, 2024
086c418
refactor gc_collect_harder
graingert Nov 21, 2024
6a76066
cleanup args/kwargs
graingert Nov 21, 2024
2cc6648
make unraisablehook last-resort exception handling as simple as possible
graingert Nov 21, 2024
77c5126
use assert_outcomes
graingert Nov 21, 2024
4741611
Update changelog/12958.improvement.rst
graingert Nov 21, 2024
a554f2a
Update src/_pytest/unraisableexception.py
graingert Nov 21, 2024
f33fb85
Update src/_pytest/unraisableexception.py
graingert Nov 21, 2024
af13058
Update src/_pytest/unraisableexception.py
graingert Nov 21, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/_pytest/tracemalloc.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
from __future__ import annotations


def tracemalloc_message(source: object) -> str:
if source is None:
return ""

try:
import tracemalloc
except ImportError:
return ""

Check warning on line 11 in src/_pytest/tracemalloc.py

View check run for this annotation

Codecov / codecov/patch

src/_pytest/tracemalloc.py#L10-L11

Added lines #L10 - L11 were not covered by tests
Copy link
Member Author

Choose a reason for hiding this comment

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

this should be covered on pypy

Copy link
Member

Choose a reason for hiding this comment

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

Leave that comment in the code? More likely for future maintainers to see it that way 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah what I mean to say is that the coverage check is failing for this branch, but it shouldn't and I don't know why


tb = tracemalloc.get_object_traceback(source)
if tb is not None:
formatted_tb = "\n".join(tb.format())
# Use a leading new line to better separate the (large) output
# from the traceback to the previous warning text.
return f"\nObject allocated at:\n{formatted_tb}"
# No need for a leading new line.
url = "https://docs.pytest.org/en/stable/how-to/capture-warnings.html#resource-warnings"
return (
"Enable tracemalloc to get traceback where the object was allocated.\n"
f"See {url} for more info."
)
195 changes: 113 additions & 82 deletions src/_pytest/unraisableexception.py
Original file line number Diff line number Diff line change
@@ -1,100 +1,131 @@
from __future__ import annotations

import collections
import functools
import gc
import sys
import traceback
from types import TracebackType
from typing import Any
from typing import Callable
from typing import Generator
from typing import NamedTuple
from typing import TYPE_CHECKING
import warnings

from _pytest.config import Config
from _pytest.tracemalloc import tracemalloc_message
import pytest


if TYPE_CHECKING:
from typing_extensions import Self


# Copied from cpython/Lib/test/support/__init__.py, with modifications.
class catch_unraisable_exception:
"""Context manager catching unraisable exception using sys.unraisablehook.

Storing the exception value (cm.unraisable.exc_value) creates a reference
cycle. The reference cycle is broken explicitly when the context manager
exits.

Storing the object (cm.unraisable.object) can resurrect it if it is set to
an object which is being finalized. Exiting the context manager clears the
stored object.

Usage:
with catch_unraisable_exception() as cm:
# code creating an "unraisable exception"
...
# check the unraisable exception: use cm.unraisable
...
# cm.unraisable attribute no longer exists at this point
# (to break a reference cycle)
"""

def __init__(self) -> None:
self.unraisable: sys.UnraisableHookArgs | None = None
self._old_hook: Callable[[sys.UnraisableHookArgs], Any] | None = None

def _hook(self, unraisable: sys.UnraisableHookArgs) -> None:
# Storing unraisable.object can resurrect an object which is being
# finalized. Storing unraisable.exc_value creates a reference cycle.
self.unraisable = unraisable

def __enter__(self) -> Self:
self._old_hook = sys.unraisablehook
sys.unraisablehook = self._hook
return self

def __exit__(
self,
exc_type: type[BaseException] | None,
exc_val: BaseException | None,
exc_tb: TracebackType | None,
) -> None:
assert self._old_hook is not None
sys.unraisablehook = self._old_hook
self._old_hook = None
del self.unraisable


def unraisable_exception_runtest_hook() -> Generator[None]:
with catch_unraisable_exception() as cm:
try:
yield
finally:
if cm.unraisable:
if cm.unraisable.err_msg is not None:
err_msg = cm.unraisable.err_msg
else:
err_msg = "Exception ignored in"
msg = f"{err_msg}: {cm.unraisable.object!r}\n\n"
msg += "".join(
traceback.format_exception(
cm.unraisable.exc_type,
cm.unraisable.exc_value,
cm.unraisable.exc_traceback,
)
)
warnings.warn(pytest.PytestUnraisableExceptionWarning(msg))
pass

if sys.version_info < (3, 11):
from exceptiongroup import ExceptionGroup


class UnraisableMeta(NamedTuple):
Copy link
Member

Choose a reason for hiding this comment

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

I'd use a dataclass; named tuples feel a bit outdated these days. But no problem if you want to keep it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have a gut feeling that NamedTuple is better here

Copy link
Member

@nicoddemus nicoddemus Nov 21, 2024

Choose a reason for hiding this comment

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

Can you elaborate? It is only being used for attribute access, never as an actual tuple.

I agree with @bluetech, in general I go with dataclass, only reaching for a NamedTuple when I'm updating an existing API that used to return a tuple. For new APIs, I prefer using a dataclass as NamedTuples have some problems:

https://codereview.doctor/features/python/best-practice/avoid-named-tuple-use-dataclass

Copy link
Member

Choose a reason for hiding this comment

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

In the end though, is not a big problem if you really want to use a NamedTuple for some reason, given it is only internal and not public API.

msg: str
cause_msg: str
exc_value: BaseException | None

@pytest.hookimpl(wrapper=True, tryfirst=True)
def pytest_runtest_setup() -> Generator[None]:
yield from unraisable_exception_runtest_hook()

_unraisable_exceptions: collections.deque[UnraisableMeta | BaseException] = (
Copy link
Member

Choose a reason for hiding this comment

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

Can we use the config stash for this instead of a global?

Of course, unraisablehook itself is a global, so it's not like we can really make this plugin safe for multiple concurrent pytest sessions. But still, I prefer to avoid globals when possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

How do I get to the config from the various test stage hooks?

Copy link
Member

Choose a reason for hiding this comment

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

They can all receive an pytest.Item, so you can access the config via item.config.

collections.deque()
)

@pytest.hookimpl(wrapper=True, tryfirst=True)
def pytest_runtest_call() -> Generator[None]:
yield from unraisable_exception_runtest_hook()

def collect_unraisable() -> None:
errors: list[pytest.PytestUnraisableExceptionWarning | RuntimeError] = []
meta = None
hook_error = None
try:
while True:
try:
meta = _unraisable_exceptions.pop()
except IndexError:
break
Comment on lines +47 to +51
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps like this? It's shorter and makes the stop condition more obvious.

Suggested change
while True:
try:
meta = _unraisable_exceptions.pop()
except IndexError:
break
while _unraisable_exceptions:
meta = _unraisable_exceptions.pop()

Copy link
Member Author

Choose a reason for hiding this comment

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

The current code only checks if the deque has an item once, and while some_deque: some_deque.pop() is an anti pattern because it's not thread safe (we don't care about it here, but we might as well use the thread safe version)


@pytest.hookimpl(wrapper=True, tryfirst=True)
def pytest_runtest_teardown() -> Generator[None]:
yield from unraisable_exception_runtest_hook()
if isinstance(meta, BaseException):
hook_error = RuntimeError("Failed to process unraisable exception")
hook_error.__cause__ = meta
errors.append(hook_error)
continue

msg = meta.msg
try:
warnings.warn(pytest.PytestUnraisableExceptionWarning(msg))
except pytest.PytestUnraisableExceptionWarning as e:
if meta.exc_value is not None:
graingert marked this conversation as resolved.
Show resolved Hide resolved
# exceptions have a better way to show the traceback, but
# warnings do not, so hide the traceback from the msg and
# set the cause so the traceback shows up in the right place
graingert marked this conversation as resolved.
Show resolved Hide resolved
e.args = (meta.cause_msg,)
e.__cause__ = meta.exc_value
errors.append(e)

if len(errors) == 1:
raise errors[0]
if errors:
raise ExceptionGroup("multiple unraisable exception warnings", errors)
finally:
del errors, meta, hook_error


def _cleanup(prev_hook: Callable[[sys.UnraisableHookArgs], object]) -> None:
try:
for i in range(5):
graingert marked this conversation as resolved.
Show resolved Hide resolved
gc.collect()
collect_unraisable()
finally:
sys.unraisablehook = prev_hook


def unraisable_hook(unraisable: sys.UnraisableHookArgs) -> None:
try:
graingert marked this conversation as resolved.
Show resolved Hide resolved
err_msg = (
"Exception ignored in" if unraisable.err_msg is None else unraisable.err_msg
)
summary = f"{err_msg}: {unraisable.object!r}"
traceback_message = "\n\n" + "".join(
traceback.format_exception(
unraisable.exc_type,
unraisable.exc_value,
unraisable.exc_traceback,
)
)
tracemalloc_tb = tracemalloc_message(unraisable.object)
msg = summary + traceback_message + tracemalloc_tb
cause_msg = summary + tracemalloc_tb

_unraisable_exceptions.append(
UnraisableMeta(
# we need to compute these strings here as they might change after
# the unraisablehook finishes and before the unraisable object is
# collected by a hook
msg=msg,
cause_msg=cause_msg,
exc_value=unraisable.exc_value,
)
)
except BaseException as e:
_unraisable_exceptions.append(e)
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this an UnraisableMeta as well, perhaps preserving the original unraisable just without the extra info?

Copy link
Member Author

Choose a reason for hiding this comment

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

this BaseException handling needs to be as simple as possible - the failures here are the sorts of thing relating to missing globals and things. I've refactored to make this branch only load locals



def pytest_configure(config: Config) -> None:
prev_hook = sys.unraisablehook
config.add_cleanup(functools.partial(_cleanup, prev_hook))
sys.unraisablehook = unraisable_hook


@pytest.hookimpl(trylast=True)
def pytest_runtest_setup() -> None:
collect_unraisable()


@pytest.hookimpl(trylast=True)
def pytest_runtest_call() -> None:
collect_unraisable()


@pytest.hookimpl(trylast=True)
def pytest_runtest_teardown() -> None:
collect_unraisable()
26 changes: 4 additions & 22 deletions src/_pytest/warnings.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from _pytest.main import Session
from _pytest.nodes import Item
from _pytest.terminal import TerminalReporter
from _pytest.tracemalloc import tracemalloc_message
import pytest


Expand Down Expand Up @@ -76,32 +77,13 @@ def catch_warnings_for_item(

def warning_record_to_str(warning_message: warnings.WarningMessage) -> str:
"""Convert a warnings.WarningMessage to a string."""
warn_msg = warning_message.message
msg = warnings.formatwarning(
str(warn_msg),
return warnings.formatwarning(
str(warning_message.message),
warning_message.category,
warning_message.filename,
warning_message.lineno,
warning_message.line,
)
if warning_message.source is not None:
try:
import tracemalloc
except ImportError:
pass
else:
tb = tracemalloc.get_object_traceback(warning_message.source)
if tb is not None:
formatted_tb = "\n".join(tb.format())
# Use a leading new line to better separate the (large) output
# from the traceback to the previous warning text.
msg += f"\nObject allocated at:\n{formatted_tb}"
else:
# No need for a leading new line.
url = "https://docs.pytest.org/en/stable/how-to/capture-warnings.html#resource-warnings"
msg += "Enable tracemalloc to get traceback where the object was allocated.\n"
msg += f"See {url} for more info."
return msg
) + tracemalloc_message(warning_message.source)


@pytest.hookimpl(wrapper=True, tryfirst=True)
Expand Down
Loading
Loading