Fix resource leaks, Perl interpreter lifecycle, and silent failures - #18
Open
quinnjr wants to merge 2 commits into
Open
Fix resource leaks, Perl interpreter lifecycle, and silent failures#18quinnjr wants to merge 2 commits into
quinnjr wants to merge 2 commits into
Conversation
Applies findings from a combined /simplify + /optimize + /audit pass
followed by a full /code-review of the resulting diff:
- Perl: interpreter is now constructed once and reused across plugin
calls instead of a full alloc/parse/destruct per invocation, with
proper error propagation (do FILE load failures now throw and are
caught by main.cxx's existing fail-fast path instead of crashing or
silently reusing the previous plugin's subs).
- PerlIO.pm: fixed two-argument open() (was a shell-injection vector),
scalar-context arg bug, and a wrong-sigil return that made
readParameters() always return nothing.
- Compiled.cxx: fixed a leaked Plugin*/dlopen handle per invocation and
mutex-guarded the new dlopen handle cache against concurrent
LitterLaunch pipelines.
- Py.cxx: mutex-guarded the new sys.path dedup cache for the same
reason; fixed a new[]/delete mismatch.
- main.cxx: plugin exceptions and "no suitable language" now abort the
pipeline (previously silent no-ops that let the run continue and
exit 0); exit(1) no longer fires directly from a LitterLaunch worker
thread, so sibling threads get to finish first.
- PluginManager.h: create() no longer null-derefs on an unregistered
plugin name; --without-{python,perl,r} now actually gates
supportedLanguages() so those builds link.
- SConstruct: --without-{python,perl,r} now also gates compilation of
the corresponding language source and linked libraries; removed a
hardcoded machine-specific JVM library path.
- R.cxx: fixed a dangling-pointer trap between unload()/load().
- Assorted: PluGen generator bugs (undeclared Java field, OOB vector
access, substring-vs-exact plugin-name match), testPluMA.py's
check() always returning True after the first line, Dockerfile
layer ordering, dead code removal, deduplicated the
checkPool.py/getPool.py/getPlugins.py scraping logic into
pool_utils.py.
Adds test coverage for previously-untested paths: a two-Perl-plugin
chain (plugins/PerlChainStepA + PerlChainStepB) exercising the
interpreter-reuse code above, and tests/test_negative_paths.sh
verifying pluma exits non-zero on an unregistered plugin name.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…n-fixes # Conflicts: # SConstruct # build_config.py # build_support.py # src/PluGen/main.cxx # src/languages/Compiled.cxx # src/main.cxx
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.
Summary
Applies findings from a combined
/simplify+/optimize+/auditpass, followed by a full multi-dimension code review of the resulting diff, then adds test coverage for the gaps that review surfaced.src/languages/Perl.cxx,Perl.h): the interpreter is now constructed once and reused across plugin calls instead of a full alloc/parse/destruct per invocation (large perf win on pipelines with multiple Perl steps). Load failures nowthrowand are caught bymain.cxx's existing fail-fast path instead of crashing the process or silently reusing the previous plugin'sinput/run/outputsubs.PerlIO.pm: fixed a two-argumentopen()(shell-injection vector via a crafted filename), a scalar-context arg-parsing bug, a wrong-sigilreturnthat madereadParameters()always return nothing, and a missing trailing1;(Perl modules must return true). Also flagged separately (not fixed here, needs a maintainer call): the module's name collides with Perl's own built-inPerlIOI/O-layer pragma, souse PerlIO;in a plugin silently resolves to the wrong module — will file as a separate issue.src/languages/Compiled.cxx: fixed a leakedPlugin*/dlopenhandle on every plugin invocation; mutex-guarded the newdlopenhandle cache against concurrentLitterLaunchpipelines.src/languages/Py.cxx: mutex-guarded the newsys.pathdedup cache for the same concurrency reason; fixed anew[]/deletemismatch.src/main.cxx: a plugin exception or "no suitable language for plugin" now aborts the pipeline (previously both were silent no-ops that let the run continue and exit 0).exit(1)no longer fires directly from aLitterLaunchworker thread — sibling threads get to finish first via a shared atomic flag checked after joins.src/PluginManager.h:create()no longer null-derefs on an unregistered plugin name;supportedLanguages()is now properly gated byHAVE_PYTHON/HAVE_PERL/HAVE_Rso--without-*builds actually link.SConstruct:--without-{python,perl,r}now also gates compilation of the corresponding language source and linked libraries (previously only affected some targets, causing link failures); removed a hardcoded, machine-specific JVM library path left over from local debugging.src/languages/R.cxx: fixed a dangling-pointer trap betweenunload()/load().testPluMA.py'scheck()returningTrueafter only checking the first line of a diff, Dockerfile layer ordering for build caching, dead-code removal, deduplicated thecheckPool.py/getPool.py/getPlugins.pyplugin-pool scraping logic into a sharedpool_utils.py.Test coverage added
plugins/PerlChainStepA+plugins/PerlChainStepB: two minimal Perl plugins chained in oneconfig.txt, the first example anywhere in the repo that runs more than one Perl plugin per process — exercises the interpreter-reuse code above. Verified locally: both plugins execute in one process, each produces its own correct output (not stale/leaked from the other), matching.expectedfiles exactly.tests/test_negative_paths.sh(wired into CI): verifies./plumaexits non-zero when a config references an unregistered plugin name, instead of the previous silent success. Verified locally against a live build.Test plan
scons, andscons --without-r) with no new compiler errors in any touched filepy_compile;PerlIO.pmand both new plugin scripts passperl -c./plumaand ran both new test fixtures against it directly — both passtestPluMA.py+ the new negative-path test) — will run automatically on this PR