github-actions[bot] commented on code in PR #67450:
URL: https://github.com/apache/doris/pull/67450#discussion_r3914423243
##########
be/src/udf/python/python_server.py:
##########
@@ -2701,22 +2701,21 @@ def _clear_modules_from_location(self, location: str)
-> list:
cleared = []
with ModuleUDFLoader._module_cache_lock:
- keys_to_remove = [
- key for key in ModuleUDFLoader._module_cache
- if key[0] == location
+ module_names_to_remove = [
Review Comment:
[P1] Fence imports by location before reporting a successful clear
This snapshot is taken before any lock shared with `_get_or_import_module`.
If a DoExchange has acquired the module-name lock and is still inside
`importlib.import_module`, the location is not in `_module_cache` yet, so this
list is empty and the clear action returns `success: true`. The importer can
then publish the old module after the caller proceeds toward deleting the
extracted directory. Please use stable per-location state that both import and
clear acquire unconditionally (or an equivalent generation/tombstone) and add a
barrier-based clear-versus-import test.
##########
be/src/udf/python/python_server.py:
##########
@@ -2701,22 +2701,21 @@ def _clear_modules_from_location(self, location: str)
-> list:
cleared = []
with ModuleUDFLoader._module_cache_lock:
- keys_to_remove = [
- key for key in ModuleUDFLoader._module_cache
- if key[0] == location
+ module_names_to_remove = [
+ module.__name__
+ for key, module in ModuleUDFLoader._module_cache.items()
+ if key == location
Review Comment:
[P2] Clear location dependencies even when the target import failed
The location scan runs only when `_module_cache` already contains the
target. If `bad_udf` imports a local `helper` and then raises, the target is
never cached but `helper` remains in `sys.modules`; this clear returns success
without scanning the location. A later UDF importing the same helper name can
therefore reuse code from the deleted UDF directory. Please track modules
introduced per location, including dependencies of failed imports, and clear
them even when no target cache entry exists, with a failed-import regression.
##########
be/test/udf/python/python_server_test.cpp:
##########
@@ -499,6 +614,75 @@ TEST_F(PythonServerTest,
ClearModuleCacheWithoutProcessesIsNoOp) {
EXPECT_TRUE(status.ok()) << status.to_string();
}
+TEST_F(PythonServerTest, ClearModuleCacheReloadsModuleOnNextUdfExecution) {
+ auto python = find_python_udf_interpreter();
+ if (!python) {
+ GTEST_SKIP() << "Python with pandas and pyarrow is required";
Review Comment:
[P2] Make the cache-key regression mandatory in its test target
This is the only test that would fail on the old string-key eviction bug,
but it silently skips unless an ambient `python3` has both pandas and pyarrow.
The BE-UT harness does not provision or set `DORIS_PYTHON_UDF_TEST_PYTHON`; the
existing DROP regressions either use another function-id-qualified location or
inline code, so they do not catch this bug. A green test run can therefore
leave the main fix unexecuted. Please provide a hermetic interpreter for this
target and fail setup if it is absent, or move the case to a Python-UDF target
where those dependencies are guaranteed.
##########
be/src/udf/python/python_server.py:
##########
@@ -2701,22 +2701,21 @@ def _clear_modules_from_location(self, location: str)
-> list:
cleared = []
with ModuleUDFLoader._module_cache_lock:
- keys_to_remove = [
- key for key in ModuleUDFLoader._module_cache
- if key[0] == location
+ module_names_to_remove = [
+ module.__name__
+ for key, module in ModuleUDFLoader._module_cache.items()
+ if key == location
]
# For each module, acquire its import lock before clearing.
# This ensures no concurrent _get_or_import_module is in progress
- # for this (location, module_name) pair.
- for key in keys_to_remove:
- _, module_name = key
+ # for this module.
+ for module_name in module_names_to_remove:
Review Comment:
[P2] Synchronize every module namespace removed by this location scan
The lock here protects only the cached target's top-level name, while the
scan below also deletes absolute-import dependencies whose top-level names can
differ. Another UDF can import or bind one of those dependencies under a
different lock, allowing it to retain stale code; it can also remove a
snapshotted name before this clear reaches `del sys.modules[...]`, causing the
action to fail with `KeyError`. Please give these `sys.modules` mutations one
coherent synchronization domain, or track and protect every affected namespace
before deletion.
--
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]
