kaxil commented on code in PR #73838:
URL: https://github.com/apache/airflow/pull/73838#discussion_r4126695281
##########
airflow-core/src/airflow/settings.py:
##########
@@ -670,6 +664,16 @@ def dispose_orm(do_log: bool = True):
AsyncSession = None
+async def dispose_async_orm() -> None:
+ """Dispose the async pool on its owning loop after closing its sessions."""
+ global async_engine, AsyncSession
+
+ if async_engine is not None:
+ await async_engine.dispose()
+ async_engine = None
Review Comment:
Do the globals need clearing here? `AsyncEngine.dispose()` swaps in a fresh
pool, so the same engine works on the next loop. I checked with aiosqlite: two
`asyncio.run()` loops that each end with `await eng.dispose()`, followed by
`eng.sync_engine.dispose()` the way the atexit `dispose_orm` does, gives
connect/close/connect/close and no MissingGreenlet. Without the reset, the `is
None` / `_configure_async_session()` branch in app.py can go. So can
`isolate_async_orm` in the api_fastapi conftest and the reconfigure calls after
dispose in test_metastore.py and test_session.py. That branch is also where the
Non-DB failures come from: the fixture nulls the engine, so the lifespan builds
a new one from the `bad_schema` URL.
##########
airflow-core/tests/unit/utils/test_session.py:
##########
@@ -58,10 +58,17 @@ def test_provide_session_with_kwargs(self):
@pytest.mark.asyncio
async def test_async_session(self):
+ from airflow import settings
Review Comment:
Could `settings` be imported at the top of the module? Nothing is circular
here (test_metastore.py and test_app.py import it at the top), and using
`settings.AsyncSession()` would let the inline `from airflow.settings import
AsyncSession` below go as well.
##########
airflow-core/src/airflow/settings.py:
##########
@@ -670,6 +664,16 @@ def dispose_orm(do_log: bool = True):
AsyncSession = None
+async def dispose_async_orm() -> None:
+ """Dispose the async pool on its owning loop after closing its sessions."""
Review Comment:
Nothing in this function closes sessions. `AsyncEngine.dispose()` closes
checked-in connections and only dereferences checked-out ones, and it works in
the lifespan because uvicorn drains requests first. Maybe "Dispose the async
pool on the event loop that owns its connections"?
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_dag_run.py:
##########
@@ -2513,15 +2511,8 @@ def
test_bulk_clear_rejects_unauthorized_dag_ids_from_request_body(self, test_cl
SimpleAuthManagerUser(username="limited-user", role="user",
teams=[]),
)
)
- with (
- mock.patch("airflow.models.revoked_token.RevokedToken.is_revoked",
return_value=False),
- TestClient(
- test_client.app,
- headers={"Authorization": f"Bearer {token}"},
- base_url=str(test_client.base_url),
- ) as limited_test_client,
- ):
- response = limited_test_client.post(
+ with
mock.patch("airflow.models.revoked_token.RevokedToken.is_revoked",
return_value=False):
Review Comment:
`test_client` already runs under this `is_revoked` patch
(`_authed_test_client` in the api_fastapi conftest), so this `with` can go now
that it no longer opens a second client. The same leftover is at L4846 here and
at L6930 and L6999 in test_task_instances.py. The header fixtures in
test_backfills, test_dag_parsing and test_dag_bundles already dropped it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
