aminghadersohi commented on code in PR #45861:
URL: https://github.com/apache/superset/pull/45861#discussion_r4476128061
##########
superset/models/core.py:
##########
@@ -1740,16 +1740,34 @@ def _evict_engine_cache(
"""Evict all cached engines for a database when it is updated or deleted.
URL/kwargs changes already produce a new cache key, so stale engines are
- never served to callers. This eviction step is purely to reclaim memory:
- without it, old engines for a renamed host or rotated password would linger
- in _ENGINE_CACHE until the process restarted.
+ never served to callers. Eviction reclaims memory (old engines for a
+ renamed host or rotated password would otherwise linger in _ENGINE_CACHE
+ until the process restarted). Engines on a ``QueuePool`` are also disposed,
+ which closes the connections their pool holds idle. Some databases, such as
+ DuckDB, allow only one configuration per database file per process, so an
+ idle connection kept by an engine with the previous settings would block
+ opening the edited ones.
+
+ Engines on pools whose disposal also closes checked-out connections
+ (``StaticPool``, ``SingletonThreadPool``) are not disposed, so that a query
+ running at that moment is not broken: their connection is released when the
+ engine is garbage collected. Connections of a ``QueuePool`` that are
checked
+ out stay on its previous pool and are not released deterministically.
+ Disposal runs after the cache lock is released, and a failure to dispose
+ never interrupts the update or delete.
"""
if target.id is None:
return
with _ENGINE_CACHE_LOCK:
stale = [k for k in _ENGINE_CACHE if k[0] == target.id]
- for k in stale:
- _ENGINE_CACHE.pop(k, None)
+ evicted = [_ENGINE_CACHE.pop(k) for k in stale]
+ for engine in evicted:
+ if not isinstance(engine.pool, QueuePool):
+ continue
+ try:
+ engine.dispose()
+ except Exception: # pylint: disable=broad-except
+ logger.warning("Failed to dispose an evicted engine",
exc_info=True)
sqla.event.listen(Database, "after_update", _evict_engine_cache)
Review Comment:
Verified fixed at fb9fd78769ba9790589578a24f15b55f38f0d88f (ffe3dc18b5 +
fb9fd78769): `_evict_engine_cache` remembers the id in `Session.info`, and the
`after_soft_rollback` listener `_evict_engine_cache_on_rollback` evicts again;
`after_commit` (`_forget_evicted_database_ids`) forgets it only on the
outermost commit.
Regression tests in tests/unit_tests/models/core_test.py:
`test_engine_evicted_again_when_update_rolls_back`,
`test_engine_evicted_again_when_update_rolls_back_after_savepoint`,
`test_engine_rollback_eviction_survives_dispose_failure` fail against the
production file at 82436aa7d7 and pass at head; dropping the listener or the
remembered id fails all three.
Siblings checked:
- superset/models/core.py:1832 — fixed in ffe3dc18b5 (`after_delete` shares
`_evict_engine_cache`, so a rolled-back delete re-evicts too)
- superset/daos/base.py:584 — not affected: `Database` is hard-deleted via
`session.delete`, so `after_delete` fires
- superset/models/core.py:710 — not affected: the prequery engine is
uncached and disposed on exit
Remaining: a session closed without a commit or rollback keeps the id and
the edited engine (the PR body lists this); `UpdateDatabaseCommand.run` rolls
back through `@transaction`.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
