Skip to content

Exit only the context managers that were entered in with and async with - #890

Open
Zedas030 wants to merge 1 commit into
custom-components:masterfrom
Zedas030:with-exit-only-entered-managers
Open

Zedas030 wants to merge 1 commit into
custom-components:masterfrom
Zedas030:with-exit-only-entered-managers

Conversation

@Zedas030

Copy link
Copy Markdown

Fixes #881

ast_with evaluated all managers first, then entered them, and on an error called the exit of every manager in the list, including the one whose enter had just raised. With an @asynccontextmanager (e.g. httpx AsyncClient.stream() on a refused connection) that turned the real error into RuntimeError: generator didn't stop after athrow().

It now follows Python's semantics for several items, i.e. nested with statements:

  • each manager is evaluated and entered in turn, and only managers that were entered are exited, innermost first
  • an exit that returns true suppresses the error, so the outer exits get None (before, every exit had to return true)
  • an exit that raises replaces the error for the outer exits, with the previous error as __context__
  • if the body finishes without an error and an inner exit raises, the outer exits still run with that error (before they were skipped)
  • CancelledError is passed to the exits too (before they got None)

This is mostly what @jfriend00 suggested in the issue, plus the last two points.

The new test_with_matches_python runs the same source through CPython and through pyscript and compares the results: 7 cases for with and the same 7 for async with (enter fails, inner or outer exit suppresses, exits raise, no error), plus a generator based manager whose enter fails. It fails on master and passes with this change.

I could only run the interpreter tests on Windows with a stand-in for hass (Home Assistant needs fcntl there), so I'd rely on CI for the full suite. In that setup the evalTests and evalTestsExceptions cases that don't need a real hass give the same results before and after this change.

One thing I ran into while writing the test: a pyscript class that subclasses another pyscript class doesn't inherit its __init__ (ACtx() takes no arguments), so the async manager in the test wraps Ctx instead of subclassing it. That's separate from this fix.

@craigbarratt craigbarratt left a comment

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.

Thanks for this, and for the CPython comparison test, which is a great way to pin down the semantics. Exiting only the managers that were entered is the right fix for #881.

I found a few places where the new code still differs from CPython, mostly because the exits now run after the except block instead of inside it. I checked each case below by running it through CPython and through pyscript with this PR.

Suggestion: nest single-item with statements recursively

Instead of collecting the error and replaying it by hand, each item could be handled like its own single-item with that wraps the rest. Every exit then runs inside a real except handler, so Python handles the active exception, __context__ chaining and the re-raise for us:

    async def ast_with(self, arg, async_attr=""):
        """Execute with statement; several items behave like nested with statements."""
        return await self.with_items(arg, arg.items, async_attr)

    async def with_items(self, arg, items, async_attr):
        """Enter the first manager, run the remaining items and body inside it, then exit it."""
        if not items:
            val = None
            for arg1 in arg.body:
                val = await self.aeval(arg1)
                if isinstance(val, EvalStopFlow):
                    break
            return val
        item = items[0]
        manager = await self.aeval(item.context_expr)
        enter_attr = f"__{async_attr}enter__"
        exit_attr = f"__{async_attr}exit__"
        enter_func = getattr(type(manager), enter_attr)
        exit_func = getattr(type(manager), exit_attr)
        value = await self.call_func(enter_func, enter_attr, manager)
        try:
            if item.optional_vars:
                await self.recurse_assign(item.optional_vars, value)
            val = await self.with_items(arg, items[1:], async_attr)
        except (asyncio.CancelledError, KeyboardInterrupt, SystemExit):
            # unlike Python, the exit can't suppress these, so tasks can always be stopped
            await self.call_func(exit_func, exit_attr, manager, *sys.exc_info())
            raise
        except Exception:
            if not await self.call_func(exit_func, exit_attr, manager, *sys.exc_info()):
                raise
            return None
        await self.call_func(exit_func, exit_attr, manager, None, None, None)
        return val

I tried this locally. It passes your test_with_matches_python unchanged and also matches CPython on the cases in points 1 and 2 below. It needs no error variable, so the reference cycle in point 4 goes away too. If you'd rather keep the current structure, points 1 and 2 still need fixing.

1. No exception is active while an exit runs

The exits now run after the except block has finished, so sys.exc_info() is (None, None, None) inside __exit__. A pyscript __exit__ that does a bare raise now fails with RuntimeError: No active exception to reraise instead of re-raising the body's error. logging.exception() / log.error(..., exc_info=True) inside an exit also loses the traceback. The old code called the exits from inside the except, so this is a regression.

2. The body's error can drop out of the __context__ chain

exc.__context__ is only set when it is None. If an exit raises from inside its own except block (e.g. except KeyError: raise IndexError), the new error's context is already the KeyError, so the body's original error is lost. CPython gives IndexError → KeyError → ZeroDivisionError, but this PR gives IndexError → KeyError.

3. Don't let exits suppress CancelledError, KeyboardInterrupt or SystemExit

CancelledError used to always propagate. With except BaseException, an exit that returns True (common for "ignore errors" managers) now swallows it. That stops task.cancel(), task.unique() and pyscript reloads from killing the task, which leaves it running. I confirmed this: with a suppressing manager around task.sleep(), cancelling the task runs the code after the with and the task keeps going.

Please still call the exits with these exceptions so resources are released, but always re-raise them (as in the sketch above). This is a deliberate difference from Python, so please add a bullet to the "Language Limitations" list in docs/reference.rst, something like:

  • A context manager's __exit__ (or __aexit__) is called when a task is cancelled, but returning a true value doesn't suppress CancelledError, KeyboardInterrupt or SystemExit the way it does in Python. This ensures pyscript tasks can always be stopped.

Tests

Please extend the CPython comparison with cases that check what sys.exc_info() sees during __exit__, an exit with a bare raise, an exit that raises a chained exception, and return/break inside the body. Also add a pyscript-only test that a suppressing manager doesn't swallow cancellation. Points 1 and 2 pass the current test, so it doesn't catch them.

The pyscript subclass __init__ issue you mentioned is unrelated; please open a separate issue for it.

if not hit_except:
for ctx in reversed(ctx_list):
await self.call_func(ctx["exit"], exit_attr, ctx["manager"], None, None, None)
except BaseException as exc: # pylint: disable=broad-exception-caught

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.

With BaseException, an exit returning True now suppresses CancelledError, so task.cancel()/task.unique() can no longer stop the task (see point 3 in the review). Please run the exits for CancelledError, KeyboardInterrupt and SystemExit but always re-raise them.

try:
if error is None:
await self.call_func(exit_func, exit_attr, manager, None, None, None)
elif await self.call_func(

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.

This runs after the except block has finished, so sys.exc_info() is empty inside __exit__. A bare raise in a pyscript __exit__ gives RuntimeError: No active exception to reraise (point 1).

error = None
except BaseException as exc: # pylint: disable=broad-exception-caught
if error is not None and exc is not error and exc.__context__ is None:
exc.__context__ = error

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.

When the exit's exception already has a __context__ (raised from inside an except in __exit__), the body's error is dropped from the chain. CPython keeps it at the end: IndexError → KeyError → ZeroDivisionError (point 2).

exc.__context__ = error
error = exc
if error is not None:
raise error

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.

Minor: re-raising a long-lived local creates a frame → exception → traceback → frame cycle, which keeps the managers alive until GC. If you keep this structure, use try: raise error / finally: error = None. The recursive version in the review avoids it.

Comment thread tests/test_unit_eval.py
import ast as ast_module
import contextlib
from dataclasses import dataclass
import types

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.

Nit: from types import ModuleType is already imported. Use from types import FunctionType, ModuleType instead of adding import types.

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.

async with calls __aexit__ even when __aenter__ raises

2 participants