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

KI protection f_locals materialization results in reference cycles on 3.12 and below #3108

Closed
graingert opened this issue Oct 13, 2024 · 2 comments · Fixed by #3110
Closed

Comments

@graingert
Copy link
Member

graingert commented Oct 13, 2024

consider:

import trio
import gc

class MyException(Exception):
    pass

async def demo():
    async def handle_error():
        try:
            raise MyException
        except MyException as e:
            exceptions.append(e)

    exceptions = []
    try:
        async with trio.open_nursery() as n:
            n.start_soon(handle_error)
        raise ExceptionGroup("errors", exceptions)
    finally:
        del exceptions

async def main():
    exc = None
    try:
        await demo()
    except* MyException as excs:
        exc = excs.exceptions[0]

    assert exc is not None
    print(gc.get_referrers(exc))
    exc.__traceback__.tb_frame.f_locals  # re-materialize f_locals to sync any deletions
    print(gc.get_referrers(exc))

trio.run(main)

this prints:

[[MyException()]]
[]

so there's a reference cycle from the exc.__traceback__.tb_frame.f_locals["exceptions"][0] is exc, which gets cleared when you re-materialize f_locals

this is caused by this materialization of the frame f_locals

coro.cr_frame.f_locals.setdefault(LOCALS_KEY_KI_PROTECTION_ENABLED, system_task)

See also agronholm/anyio#809

@graingert
Copy link
Member Author

I can work around this by passing ExceptionGroup a copy() of exceptions, and calling exceptions.clear()

@graingert graingert changed the title KI protection f_locals materialization results in reference cycles KI protection f_locals materialization results in reference cycles on 3.12 and below Oct 13, 2024
graingert added a commit to graingert/trio that referenced this issue Oct 13, 2024
@graingert
Copy link
Member Author

a nice simple test that shows the problem here:

async def test_ki_protection_check_does_not_freeze_locals() -> None:
    class A:
        pass

    a = A()
    wr_a = weakref.ref(a)
    assert not _core.currently_ki_protected()
    del a
    assert wr_a() is None

graingert added a commit that referenced this issue Oct 27, 2024
* gh-3108: avoid materializing f_locals by using weakrefs to code objects instead

* enable ki protection on async_generator objects

* avoid adding an extra coroutine wrapper to Task coros

* fix returning the wrong object in (enable|disable)_ki_protection

* remove KIProtectionSignature from _check_type_completeness.json

* fix refcycles

* add newsfragment

* fix mypy

* now that the type annotation for enable_ki_protection is fixed, we need to fix the use of Any

* pre-commit

* add test for ki protection leaking accross local functions

* add fix for ki protection leaking accross local functions

* do slots properly

* python 3.8 support

* test reading currently_ki_protected doesn't freeze locals

* cover some tricky bits of ki.py

* cover a potentially impossible scenario

* eek out some last coverage of the eeking out coverage tests

* even more partial coverage

* Update src/trio/_core/_ki.py

* cleaner _IdRef.__eq__

* if the current_task().coro.cr_frame is in the stack ki_protection_enabled is current_task()._ki_protected

* Update newsfragments/3108.bugfix.rst

* avoid copying code objects for ki protected function

* Update src/trio/_core/_ki.py

* Update src/trio/_core/_ki.py

Co-authored-by: EXPLOSION <git@helvetica.moe>

* remove workaround for 3.8

* Add docs and update news

Co-Authored-By: oremanj <oremanj@gmail.com>

* wrap ki protection locals demos in async def so they work

* add newsfragment for 2670

* Apply suggestions from code review

* use peo614

* add tests for passing on inspect flags

* 'return; yield' isn't considered covered

* Update newsfragments/3108.bugfix.rst

* [pre-commit.ci] auto fixes from pre-commit.com hooks

---------

Co-authored-by: EXPLOSION <git@helvetica.moe>
Co-authored-by: oremanj <oremanj@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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 a pull request may close this issue.

1 participant