Protect Python interpreter on SeqDB teardown - #8
Open
sarsaeroth wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #5 .
Pando supports loading Cython-generated Python filters dynamically through add_filter_dir and install_filter. The filter .sos produced from
python_filter_template/python_template.cppare single-phase initialized Cython modules, which means theirPyInit_<name>function executes once per process to register the module in the running interpreter'ssys.modules. Once registered, the module is owned by that specific interpreter instance and cannot be reattached to a different one.SeqDB::~SeqDB()currently callsPy_FinalizeEx, fully tearing down the interpreter. After destruction, any subsequentSeqDBconstructor callsPy_Initializefor a fresh interpreter, but the filter.sos loaded in the process still hold onto the static state from the previousPyInitexecution which is no longer valid. CallingPyInit_<name>a second time becomes a NOP or returnsNULL, the module is never registered in the new interpreter, andPyImport_ImportModule(filter_name)fails withModuleNotFoundError. The cascadingcython_failtag prevents the filter from running, and the failure is silently swallowed rather than surfaced. The upstream code already acknowledges this failure mode with the warning"could not import module, might mean it was already imported"inpython_template.cpp, which identifies the symptom but not the root cause. This restricts Pando's filter system in a way that's not documented or surfaced to users: filters cannot be reused acrossSeqDBinstances in the same process. Any architecture that creates a newSeqDBafter disposing of an earlier one loses access to its filters silently. Production daemons that need to rebuild database state without a full process restart hit this.Changes:
code/include/seq_db.hppL dropPy_FinalizeExfrom~SeqDB(). The interpreter persists for the process lifetime, which is the correct pattern when single-phase Cython modules are dynamically loaded as filters.Py_Initializeis documented as idempotent when the interpreter is already running, so existing constructor calls remain correct. Memory-leak implication is bounded; interpreter resources are reclaimed by process exit, which is when most embedders rely on them being freed.code/python_filter_template/python_template.cpp: guardPyInit_<filter_name>with asys.modulescheck. The unconditionalPyInitcalls inrun_()andshould_run()are replaced by calls to a new helper that first queriessys.modulesviaPyImport_GetModuleDictandPyDict_GetItemString. If the module is already registered, skipPyInit; otherwise call it. Even within a single interpreter, callingPyInit_<name>more than once for a single-phase module has implementation-defined behavior which should be preserverd. Each filter.sois a separate translation unit, so the helper's per-filter scope is correct.