all teardown callbacks are called despite errors (#5928)

CHANGES.rst

@@ -14,6 +14,7 @@ Unreleased
     If subclasses were overriding these methods, the old signature is detected,
     shows a deprecation warning, and will continue to work during the
     deprecation period. :issue:`5815`
+-   All teardown callbacks are called, even if any raise an error. :pr:`5928`
 -   The ``should_ignore_error`` is deprecated. Handle errors as needed in
     teardown handlers instead. :issue:`5816`
 -   ``template_filter``, ``template_test``, and ``template_global`` decorators

docs/appcontext.rst

@@ -163,7 +163,8 @@ The teardown callbacks are called by the context when it is popped. They are
 called even if there is an unhandled exception during dispatch. They may be
 called multiple times in some test scenarios. This means there is no guarantee
 that any other parts of the request dispatch have run. Be sure to write these
-functions in a way that does not depend on other callbacks and will not fail.
+functions in a way that does not depend on other callbacks. All callbacks are
+called even if any raise an error.
 
 
 How the Context Works

src/flask/app.py

@@ -36,6 +36,7 @@ from .globals import app_ctx
 from .globals import g
 from .globals import request
 from .globals import session
+from .helpers import _CollectErrors
 from .helpers import get_debug_flag
 from .helpers import get_flashed_messages
 from .helpers import get_load_dotenv
@@ -1430,15 +1431,24 @@ class Flask(App):
         :param exc: An unhandled exception raised while dispatching the request.
             Passed to each teardown function.
 
+        .. versionchanged:: 3.2
+            All callbacks are called rather than stopping on the first error.
+
         .. versionchanged:: 0.9
             Added the ``exc`` argument.
         """
+        collect_errors = _CollectErrors()
+
         for name in chain(ctx.request.blueprints, (None,)):
             if name in self.teardown_request_funcs:
                 for func in reversed(self.teardown_request_funcs[name]):
-                    self.ensure_sync(func)(exc)
+                    with collect_errors:
+                        self.ensure_sync(func)(exc)
 
-        request_tearing_down.send(self, _async_wrapper=self.ensure_sync, exc=exc)
+        with collect_errors:
+            request_tearing_down.send(self, _async_wrapper=self.ensure_sync, exc=exc)
+
+        collect_errors.raise_any("Errors during request teardown")
 
     def do_teardown_appcontext(
         self, ctx: AppContext, exc: BaseException | None = None
@@ -1452,12 +1462,21 @@ class Flask(App):
         :param exc: An unhandled exception raised while the context was active.
             Passed to each teardown function.
 
+        .. versionchanged:: 3.2
+            All callbacks are called rather than stopping on the first error.
+
         .. versionadded:: 0.9
         """
-        for func in reversed(self.teardown_appcontext_funcs):
-            self.ensure_sync(func)(exc)
+        collect_errors = _CollectErrors()
 
-        appcontext_tearing_down.send(self, _async_wrapper=self.ensure_sync, exc=exc)
+        for func in reversed(self.teardown_appcontext_funcs):
+            with collect_errors:
+                self.ensure_sync(func)(exc)
+
+        with collect_errors:
+            appcontext_tearing_down.send(self, _async_wrapper=self.ensure_sync, exc=exc)
+
+        collect_errors.raise_any("Errors during app teardown")
 
     def app_context(self) -> AppContext:
         """Create an :class:`.AppContext`. When the context is pushed,

src/flask/ctx.py

@@ -10,6 +10,7 @@ from werkzeug.routing import MapAdapter
 
 from . import typing as ft
 from .globals import _cv_app
+from .helpers import _CollectErrors
 from .signals import appcontext_popped
 from .signals import appcontext_pushed
 
@@ -482,16 +483,26 @@ class AppContext:
         if self._push_count > 0:
             return
 
-        try:
-            if self._request is not None:
+        collect_errors = _CollectErrors()
+
+        if self._request is not None:
+            with collect_errors:
                 self.app.do_teardown_request(self, exc)
+
+            with collect_errors:
                 self._request.close()
-        finally:
+
+        with collect_errors:
             self.app.do_teardown_appcontext(self, exc)
-            _cv_app.reset(self._cv_token)
-            self._cv_token = None
+
+        _cv_app.reset(self._cv_token)
+        self._cv_token = None
+
+        with collect_errors:
             appcontext_popped.send(self.app, _async_wrapper=self.app.ensure_sync)
 
+        collect_errors.raise_any("Errors during context teardown")
+
     def __enter__(self) -> te.Self:
         self.push()
         return self

src/flask/helpers.py

@@ -7,6 +7,7 @@ import typing as t
 from datetime import datetime
 from functools import cache
 from functools import update_wrapper
+from types import TracebackType
 
 import werkzeug.utils
 from werkzeug.exceptions import abort as _wz_abort
@@ -636,3 +637,34 @@ def _split_blueprint_path(name: str) -> list[str]:
         out.extend(_split_blueprint_path(name.rpartition(".")[0]))
 
     return out
+
+
+class _CollectErrors:
+    """A context manager that records and silences an error raised within it.
+    Used to run all teardown functions, then raise any errors afterward.
+    """
+
+    def __init__(self) -> None:
+        self.errors: list[BaseException] = []
+
+    def __enter__(self) -> None:
+        pass
+
+    def __exit__(
+        self,
+        exc_type: type[BaseException] | None,
+        exc_val: BaseException | None,
+        exc_tb: TracebackType | None,
+    ) -> bool:
+        if exc_val is not None:
+            self.errors.append(exc_val)
+
+        return True
+
+    def raise_any(self, message: str) -> None:
+        """Raise if any errors were collected."""
+        if self.errors:
+            if sys.version_info >= (3, 11):
+                raise BaseExceptionGroup(message, self.errors)  # noqa: F821
+            else:
+                raise self.errors[0]

tests/test_appctx.py

@@ -1,7 +1,10 @@
+import sys
+
 import pytest
 
 import flask
 from flask.globals import app_ctx
+from flask.testing import FlaskClient
 
 
 def test_basic_url_generation(app):
@@ -208,3 +211,55 @@ def test_clean_pop(app):
 
     assert called == ["flask_test", "TEARDOWN"]
     assert not flask.current_app
+
+
+def test_robust_teardown(app: flask.Flask, client: FlaskClient) -> None:
+    count = 0
+
+    @app.teardown_request
+    def request_teardown(e: Exception | None) -> None:
+        nonlocal count
+        count += 1
+        raise ValueError("request_teardown")
+
+    @app.teardown_appcontext
+    def app_teardown(e: Exception | None) -> None:
+        nonlocal count
+        count += 1
+        raise ValueError("app_teardown")
+
+    @app.get("/")
+    def index() -> str:
+        return ""
+
+    def request_signal(sender: flask.Flask, exc: Exception | None) -> None:
+        nonlocal count
+        count += 1
+        raise ValueError("request_signal")
+
+    def app_signal(sender: flask.Flask, exc: Exception | None) -> None:
+        nonlocal count
+        count += 1
+        raise ValueError("app_signal")
+
+    with (
+        flask.request_tearing_down.connected_to(request_signal, app),
+        flask.appcontext_tearing_down.connected_to(app_signal, app),
+    ):
+        if sys.version_info >= (3, 11):
+            with pytest.raises(ExceptionGroup, match="context teardown") as exc_info:  # noqa: F821
+                client.get()
+
+            assert len(exc_info.value.exceptions) == 2
+            eg1, eg2 = exc_info.value.exceptions
+            assert isinstance(eg1, ExceptionGroup)  # noqa: F821
+            assert "request teardown" in eg1.message
+            assert len(eg1.exceptions) == 2
+            assert isinstance(eg2, ExceptionGroup)  # noqa: F821
+            assert "app teardown" in eg2.message
+            assert len(eg2.exceptions) == 2
+        else:
+            with pytest.raises(ValueError, match="request_teardown"):
+                client.get()
+
+    assert count == 4

tests/test_basic.py

@@ -1420,20 +1420,21 @@ def test_url_for_passes_special_values_to_build_error_handler(app):
 
 
 def test_static_files(app, client):
-    rv = client.get("/static/index.html")
-    assert rv.status_code == 200
-    assert rv.data.strip() == b"<h1>Hello World!</h1>"
-    with app.test_request_context():
-        assert flask.url_for("static", filename="index.html") == "/static/index.html"
-    rv.close()
+    with client.get("/static/index.html") as rv:
+        assert rv.status_code == 200
+        assert rv.data.strip() == b"<h1>Hello World!</h1>"
+        with app.test_request_context():
+            assert (
+                flask.url_for("static", filename="index.html") == "/static/index.html"
+            )
 
 
 def test_static_url_path():
     app = flask.Flask(__name__, static_url_path="/foo")
     app.testing = True
-    rv = app.test_client().get("/foo/index.html")
-    assert rv.status_code == 200
-    rv.close()
+
+    with app.test_client().get("/foo/index.html") as rv:
+        assert rv.status_code == 200
 
     with app.test_request_context():
         assert flask.url_for("static", filename="index.html") == "/foo/index.html"
@@ -1442,9 +1443,9 @@ def test_static_url_path():
 def test_static_url_path_with_ending_slash():
     app = flask.Flask(__name__, static_url_path="/foo/")
     app.testing = True
-    rv = app.test_client().get("/foo/index.html")
-    assert rv.status_code == 200
-    rv.close()
+
+    with app.test_client().get("/foo/index.html") as rv:
+        assert rv.status_code == 200
 
     with app.test_request_context():
         assert flask.url_for("static", filename="index.html") == "/foo/index.html"
@@ -1452,25 +1453,25 @@ def test_static_url_path_with_ending_slash():
 
 def test_static_url_empty_path(app):
     app = flask.Flask(__name__, static_folder="", static_url_path="")
-    rv = app.test_client().open("/static/index.html", method="GET")
-    assert rv.status_code == 200
-    rv.close()
+
+    with app.test_client().open("/static/index.html", method="GET") as rv:
+        assert rv.status_code == 200
 
 
 def test_static_url_empty_path_default(app):
     app = flask.Flask(__name__, static_folder="")
-    rv = app.test_client().open("/static/index.html", method="GET")
-    assert rv.status_code == 200
-    rv.close()
+
+    with app.test_client().open("/static/index.html", method="GET") as rv:
+        assert rv.status_code == 200
 
 
 def test_static_folder_with_pathlib_path(app):
     from pathlib import Path
 
     app = flask.Flask(__name__, static_folder=Path("static"))
-    rv = app.test_client().open("/static/index.html", method="GET")
-    assert rv.status_code == 200
-    rv.close()
+
+    with app.test_client().open("/static/index.html", method="GET") as rv:
+        assert rv.status_code == 200
 
 
 def test_static_folder_with_ending_slash():
@@ -1487,9 +1488,10 @@ def test_static_folder_with_ending_slash():
 def test_static_route_with_host_matching():
     app = flask.Flask(__name__, host_matching=True, static_host="example.com")
     c = app.test_client()
-    rv = c.get("http://example.com/static/index.html")
-    assert rv.status_code == 200
-    rv.close()
+
+    with c.get("http://example.com/static/index.html") as rv:
+        assert rv.status_code == 200
+
     with app.test_request_context():
         rv = flask.url_for("static", filename="index.html", _external=True)
         assert rv == "http://example.com/static/index.html"

tests/test_blueprints.py

@@ -184,12 +184,12 @@ def test_templates_and_static(test_apps):
     assert rv.data == b"Hello from the Admin"
     rv = client.get("/admin/index2")
     assert rv.data == b"Hello from the Admin"
-    rv = client.get("/admin/static/test.txt")
-    assert rv.data.strip() == b"Admin File"
-    rv.close()
-    rv = client.get("/admin/static/css/test.css")
-    assert rv.data.strip() == b"/* nested file */"
-    rv.close()
+
+    with client.get("/admin/static/test.txt") as rv:
+        assert rv.data.strip() == b"Admin File"
+
+    with client.get("/admin/static/css/test.css") as rv:
+        assert rv.data.strip() == b"/* nested file */"
 
     # try/finally, in case other tests use this app for Blueprint tests.
     max_age_default = app.config["SEND_FILE_MAX_AGE_DEFAULT"]
@@ -198,10 +198,10 @@ def test_templates_and_static(test_apps):
         if app.config["SEND_FILE_MAX_AGE_DEFAULT"] == expected_max_age:
             expected_max_age = 7200
         app.config["SEND_FILE_MAX_AGE_DEFAULT"] = expected_max_age
-        rv = client.get("/admin/static/css/test.css")
-        cc = parse_cache_control_header(rv.headers["Cache-Control"])
-        assert cc.max_age == expected_max_age
-        rv.close()
+
+        with client.get("/admin/static/css/test.css") as rv:
+            cc = parse_cache_control_header(rv.headers["Cache-Control"])
+            assert cc.max_age == expected_max_age
     finally:
         app.config["SEND_FILE_MAX_AGE_DEFAULT"] = max_age_default
 

tests/test_helpers.py

@@ -32,45 +32,39 @@ class PyBytesIO:
 
 class TestSendfile:
     def test_send_file(self, app, req_ctx):
-        rv = flask.send_file("static/index.html")
-        assert rv.direct_passthrough
-        assert rv.mimetype == "text/html"
-
         with app.open_resource("static/index.html") as f:
-            rv.direct_passthrough = False
-            assert rv.data == f.read()
+            expect = f.read()
 
-        rv.close()
+        with flask.send_file("static/index.html") as rv:
+            assert rv.direct_passthrough
+            assert rv.mimetype == "text/html"
+            rv.direct_passthrough = False
+            assert rv.data == expect
 
     def test_static_file(self, app, req_ctx):
         # Default max_age is None.
 
         # Test with static file handler.
-        rv = app.send_static_file("index.html")
-        assert rv.cache_control.max_age is None
-        rv.close()
+        with app.send_static_file("index.html") as rv:
+            assert rv.cache_control.max_age is None
 
         # Test with direct use of send_file.
-        rv = flask.send_file("static/index.html")
-        assert rv.cache_control.max_age is None
-        rv.close()
+        with flask.send_file("static/index.html") as rv:
+            assert rv.cache_control.max_age is None
 
         app.config["SEND_FILE_MAX_AGE_DEFAULT"] = 3600
 
         # Test with static file handler.
-        rv = app.send_static_file("index.html")
-        assert rv.cache_control.max_age == 3600
-        rv.close()
+        with app.send_static_file("index.html") as rv:
+            assert rv.cache_control.max_age == 3600
 
         # Test with direct use of send_file.
-        rv = flask.send_file("static/index.html")
-        assert rv.cache_control.max_age == 3600
-        rv.close()
+        with flask.send_file("static/index.html") as rv:
+            assert rv.cache_control.max_age == 3600
 
         # Test with pathlib.Path.
-        rv = app.send_static_file(FakePath("index.html"))
-        assert rv.cache_control.max_age == 3600
-        rv.close()
+        with app.send_static_file(FakePath("index.html")) as rv:
+            assert rv.cache_control.max_age == 3600
 
         class StaticFileApp(flask.Flask):
             def get_send_file_max_age(self, filename):
@@ -80,23 +74,21 @@ class TestSendfile:
 
         with app.test_request_context():
             # Test with static file handler.
-            rv = app.send_static_file("index.html")
-            assert rv.cache_control.max_age == 10
-            rv.close()
+            with app.send_static_file("index.html") as rv:
+                assert rv.cache_control.max_age == 10
 
             # Test with direct use of send_file.
-            rv = flask.send_file("static/index.html")
-            assert rv.cache_control.max_age == 10
-            rv.close()
+            with flask.send_file("static/index.html") as rv:
+                assert rv.cache_control.max_age == 10
 
     def test_send_from_directory(self, app, req_ctx):
         app.root_path = os.path.join(
             os.path.dirname(__file__), "test_apps", "subdomaintestmodule"
         )
-        rv = flask.send_from_directory("static", "hello.txt")
-        rv.direct_passthrough = False
-        assert rv.data.strip() == b"Hello Subdomain"
-        rv.close()
+
+        with flask.send_from_directory("static", "hello.txt") as rv:
+            rv.direct_passthrough = False
+            assert rv.data.strip() == b"Hello Subdomain"
 
 
 class TestUrlFor:
@@ -319,15 +311,17 @@ class TestStreaming:
 
         # response is closed without reading stream
         client.get().close()
+
         # response stream is read
-        assert client.get().text == "flask"
+        with client.get() as rv:
+            assert rv.text == "flask"
 
         # same as above, but with client context preservation
         with client:
             client.get().close()
 
-        with client:
-            assert client.get().text == "flask"
+        with client, client.get() as rv:
+            assert rv.text == "flask"
 
 
 class TestHelpers:

tests/test_testing.py

@@ -76,7 +76,6 @@ def test_client_open_environ(app, client, request):
         return flask.request.remote_addr
 
     builder = EnvironBuilder(app, path="/index", method="GET")
-    request.addfinalizer(builder.close)
 
     rv = client.open(builder)
     assert rv.data == b"127.0.0.1"