[Fix](python udf) Fix module cache cleanup and error propagation - #67450
[Fix](python udf) Fix module cache cleanup and error propagation#67450linrrzqqq wants to merge 3 commits into
Conversation
Python module cache entries use location strings as keys, but cleanup treated each key as a tuple. Match the full location key and obtain the module name from the cached module before eviction.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes on head e25d4f4e31117904f4bf3507e2dedf466c5aa448.
The ordinary sequential key == location eviction and the new success:false parsing are directionally correct, but four gaps keep the cleanup contract from being reliable:
- A clear can report success while an import for that location is still before cache publication, allowing file deletion to race the in-flight query and a late stale cache entry to appear.
- Dependencies imported successfully before a target module fails are never associated with the location, so DROP cannot clear them.
- The path-based dependency scan mutates module namespaces outside the held import-lock domain, allowing another loader to retain stale code or make the clear fail.
- The only test that would fail on the old string-key bug can silently skip in the standard BE-UT harness; the existing DROP regressions do not exercise that exact failure.
Checkpoint summary: the diff is focused, and scalar UDF, UDAF, and UDTF module paths converge on the reviewed loader. The material problems are concurrency and module/file lifecycle; no separate configuration, persisted-format, FE/BE compatibility, transaction, data-write, static-lifetime, or hot-path performance regression was found. The JSON parser rejects empty, malformed, missing, wrong-type, and false success results, and no additional supported-action payload incompatibility was found. Existing and added tests were inspected, but no build or test was run because the authoritative review instructions prohibit them. No additional user review focus was supplied.
| keys_to_remove = [ | ||
| key for key in ModuleUDFLoader._module_cache | ||
| if key[0] == location | ||
| module_names_to_remove = [ |
There was a problem hiding this 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.
| module_names_to_remove = [ | ||
| module.__name__ | ||
| for key, module in ModuleUDFLoader._module_cache.items() | ||
| if key == location |
There was a problem hiding this 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.
| for key in keys_to_remove: | ||
| _, module_name = key | ||
| # for this module. | ||
| for module_name in module_names_to_remove: |
There was a problem hiding this 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.
| TEST_F(PythonServerTest, ClearModuleCacheReloadsModuleOnNextUdfExecution) { | ||
| auto python = find_python_udf_interpreter(); | ||
| if (!python) { | ||
| GTEST_SKIP() << "Python with pandas and pyarrow is required"; |
There was a problem hiding this 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.
2 participants
What problem does this PR solve?
Related PR: #61280
Problem Summary:
This PR fixes two issues in Python UDF module cache cleanup:
The Python module cache uses the complete UDF location string as its key, but the cleanup logic incorrectly treated the key as a tuple, introduced by: [Fix](pyudf) Fix concurrent race condition when import module #61280. As a result,
DROP FUNCTIONcould fail to remove the cached module, and subsequent executions could continue using stale Python code.The BE only checked whether an Arrow Flight action returned a result successfully. It did not inspect the JSON payload returned by the Python server, so a response such as
{"success": false, "error": "..."}was incorrectly counted as successful.The fix:
successto betrue.Release note
Fix Python UDF module cache cleanup and properly report Python server cleanup failures.