Skip to content

Protect Python interpreter on SeqDB teardown - #8

Open
sarsaeroth wants to merge 1 commit into
sandialabs:mainfrom
sarsaeroth:bf-5
Open

Protect Python interpreter on SeqDB teardown#8
sarsaeroth wants to merge 1 commit into
sandialabs:mainfrom
sarsaeroth:bf-5

Conversation

@sarsaeroth

Copy link
Copy Markdown

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.cpp are single-phase initialized Cython modules, which means their PyInit_<name> function executes once per process to register the module in the running interpreter's sys.modules. Once registered, the module is owned by that specific interpreter instance and cannot be reattached to a different one. SeqDB::~SeqDB() currently calls Py_FinalizeEx, fully tearing down the interpreter. After destruction, any subsequent SeqDB constructor calls Py_Initialize for a fresh interpreter, but the filter .sos loaded in the process still hold onto the static state from the previous PyInit execution which is no longer valid. Calling PyInit_<name> a second time becomes a NOP or returns NULL, the module is never registered in the new interpreter, and PyImport_ImportModule(filter_name) fails with ModuleNotFoundError. The cascading cython_fail tag 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" in python_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 across SeqDB instances in the same process. Any architecture that creates a new SeqDB after 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 drop Py_FinalizeEx from ~SeqDB(). The interpreter persists for the process lifetime, which is the correct pattern when single-phase Cython modules are dynamically loaded as filters. Py_Initialize is 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: guard PyInit_<filter_name> with a sys.modules check. The unconditional PyInit calls in run_() and should_run() are replaced by calls to a new helper that first queries sys.modules via PyImport_GetModuleDict and PyDict_GetItemString. If the module is already registered, skip PyInit; otherwise call it. Even within a single interpreter, calling PyInit_<name> more than once for a single-phase module has implementation-defined behavior which should be preserverd. Each filter .so is a separate translation unit, so the helper's per-filter scope is correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug fix: Python filters fail on second use after SeqDB destruction tears down the Python interpreter

1 participant