[Fix](python udf) isolate module caches across functions - #67511
[Fix](python udf) isolate module caches across functions#67511linrrzqqq wants to merge 3 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
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.
aac5d48 to
fef0246
Compare
fef0246 to
d8b55d1
Compare
|
/review |
|
|
||
| // lib_file changes from the downloaded zip path to the extracted directory | ||
| // while an entry is loaded. Wait for that transition before clearing Python. | ||
| std::unique_lock<std::mutex> load_lock(entry->load_lock); |
There was a problem hiding this comment.
drop_function_cache now leaves the entry in _entry_map while waiting on load_lock and broadcasting the Python clear. A concurrent _get_cache_entry can capture that entry, return its already-loaded lib_file, and then race with the erase/should_delete_library path; once shared references drain, UserFunctionCacheEntry::~UserFunctionCacheEntry deletes the directory/zip even though the caller has just received the path. This can make a concurrent UDF/UDTF (and JAR/SO lookup) fail opening its library. Please mark the entry unavailable to new lookups before waiting (or add a tombstone/retry check under _cache_lock) and coordinate replacement so a retiring entry cannot return a path that cleanup removes.
| if (drop_by_function_id) { | ||
| UserFunctionCache::instance()->drop_function_cache(clean_req.function_id); | ||
| auto status = UserFunctionCache::instance()->drop_function_cache(clean_req.function_id); | ||
| if (!status.ok()) { |
There was a problem hiding this comment.
drop_function_cache intentionally keeps the entry/files when clear_module_cache fails, but this callback only logs the error and returns from a void worker task. DropFunctionCommand submits CleanUDFCacheTask once, and the task pool has no retry/requeue or completion failure propagation, so a transiently dead Python process permanently leaks the cache entry and both files. Please add a deferred/bounded retry (or another recovery queue) and surface an unrecoverable failure instead of treating this task as handled.
| return cls._import_locks[cache_key] | ||
| @classmethod | ||
| @contextmanager | ||
| def _shared_module_import(cls): |
There was a problem hiding this comment.
temporarily_restore_udf_modules overlays udf_modules but leaves unrelated names already present in sys.modules. If an entry module performs a later import dep for a dependency that was not imported during initial load, the wrapper misses the UDF cache and the original importer returns the server's preloaded sys.modules['dep'] without consulting this UDF's sys.path. This silently mixes environments (or raises on a missing attribute). Please mask/save conflicting target names during a UDF miss (or use location-qualified module namespaces) and add a lazy absolute-dependency collision test.
| cls._module_import_condition.notify_all() | ||
|
|
||
| @classmethod | ||
| @contextmanager |
There was a problem hiding this comment.
use_module_context only binds a ContextVar and does not retain a reader/borrower lease. _handle_exchange_udf applies it separately around each batch, so a paused stream can have its module cache evicted by clear_module_cache and its directory deleted by drop_function_cache between batches. If the next batch lazily imports a dependency, the wrapper restores a path that no longer exists and the running query fails. Please retain a module-context/read lease for the exchange lifetime (or defer eviction/deletion until the stream closes).
| ) | ||
| _current_module_import_operation: contextvars.ContextVar[ | ||
| Optional[_ModuleImportOperation] | ||
| ] = contextvars.ContextVar("current_module_import_operation", default=None) |
There was a problem hiding this comment.
The child-context capture relies on thread_target is builtins.__import__ or thread_target is importlib.import_module. A valid UDF pattern such as Thread(target=functools.partial(importlib.import_module, '.dep', package)) therefore starts with no direct_import_context; the partial invokes the wrapper without UDF caller globals and the dependency is searched only in the server environment, producing a wrong module or ModuleNotFoundError. Please propagate the owning context to adapted callables (or wrap the callable invocation) rather than relying on identity checks.
What problem does this PR solve?
Fixes Python UDF module isolation and cache cleanup issues when different functions contain modules with the same name.
Python UDF/UDAF/UDTF functions run in shared Python server processes and therefore share process-wide import state such as
sys.modulesandsys.path. When two function packages contain dependencies with the same module name, one function may reuse the module loaded from another package. This is especially visible with:Cache cleanup failures could also be ignored by the BE, allowing module files to be deleted while Python processes still referenced them.
What is changed?
__import__andimportlib.import_moduleso delayed imports use the correct function package.sys.path/sys.modulesmodifications with a reader-writer locking scheme.sys.modulesscan for cached imports.UserFunctionCache.