fix: post-DESTROY/weaken regressions + DBIx::Class Phase 9 re-baseline#485
Open
fix: post-DESTROY/weaken regressions + DBIx::Class Phase 9 re-baseline#485
Conversation
Two regressions from the DESTROY/weaken merge (PR #464): 1. BytecodeInterpreter: SCOPE_EXIT_CLEANUP_ARRAY/HASH/scalar opcodes crash with ClassCastException when the interpreter fallback path reuses registers with unexpected types. Add instanceof guards before casting. Fixes Sub::Exporter::Progressive (used by Devel::GlobalDestruction, needed by DBIx::Class). 2. GlobalDestruction: runGlobalDestruction() iterates global variable HashMaps while DESTROY callbacks can modify them, causing ConcurrentModificationException. Snapshot collections with toArray() before iterating. Fixes DBIx::Class Makefile.PL. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…weaken - Updated branch/PR references for feature/dbix-class-destroy-weaken - Added Phase 9 section documenting post-DESTROY/weaken assessment - Documented 645 ok / 183 not ok across 92 test files - Identified premature DESTROY blocker (20 tests) and GC leak blocker - Catalogued improvements from DESTROY/weaken merge (PR #464) - Updated Next Steps with new priorities (P0-P2) - Marked obsoleted items (Phase 7, old GC/DESTROY sections) Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ings Add localBindingExists flag to RuntimeBase that tracks when a named hash/array (my %hash, my @array) has had a reference created via the \ operator. This flag indicates a JVM local variable slot holds a strong reference not counted in refCount. When refCount reaches 0, the flag prevents premature callDestroy since the local variable may still be alive. The flag is cleared at scope exit (scopeExitCleanupHash/Array), allowing subsequent refCount==0 to correctly trigger callDestroy. This fixes the DBIx::Class bug where \%reg stored via accessor caused premature DESTROY of Schema objects when %reg went out of scope, even though the hash was still alive through the stored reference. The localBindingExists check is applied consistently across all refCount decrement paths: setLargeRefCounted, undefine, weaken, MortalList.flush, and MortalList.popAndFlush. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
fglock
force-pushed
the
feature/dbix-class-destroy-weaken
branch
from
6586ec9 to
3b9bb81
Compare
Sub::Exporter::Progressive's import() relies on caller() to determine
the target package. When Sub::Uplevel overrides CORE::GLOBAL::caller
(used by Test::Exception via Test::Builder), PerlOnJava's caller()
returns wrong frames during `use` processing, causing SEP to install
exports into the wrong package. This prevented in_global_destruction
from being exported to Devel::GlobalDestruction, which namespace::clean
then removed, causing a bareword error in DESTROY methods.
Fix by bundling a simplified Devel::GlobalDestruction that uses plain
Exporter instead of Sub::Exporter::Progressive. Since PerlOnJava always
has ${^GLOBAL_PHASE} (Perl 5.14+ feature), the implementation is a
simple one-liner.
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Add the complete DBI::Const module hierarchy needed by DBIx::Class: - DBI::Const::GetInfo::ANSI - ANSI SQL/CLI constants (from DBI 1.643) - DBI::Const::GetInfo::ODBC - ODBC constants (from DBI 1.643) - DBI::Const::GetInfoType - merged name-to-number mapping - DBI::Const::GetInfoReturn - upgraded from stub to real implementation These are pure Perl constant-data modules from the DBI distribution. DBIx::Class uses them to translate info type names (e.g. SQL_DBMS_NAME) to numeric codes for $dbh->get_info(), which our JDBC-based DBI already implements with matching numeric codes. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
314-test analysis: 155 blocked by "detached result source" (weak ref cleared during clone -> _copy_state_from), ~10 GC-only, ~25 real+GC, ~6 errors. Root cause traced to Schema->connect's shift->clone->connection chain where the clone temporary's refCount drops to 0 mid-operation. Added reference to dev/architecture/weaken-destroy.md for refCount internals needed for debugging. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Corrected categorization: 27 GC-only (was ~10), only 4 real functional failures across all 40 non-detached test files. Added DESTROY trace confirming Schema::DESTROY fires during _copy_state_from in clone(). Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Prevents premature DESTROY of return values from chained method calls like shift->clone->connection(@_). During list assignment materialization (my ($self, @info) = @_), setLargeRefCounted calls MortalList.flush() which processes pending decrements from inner scope exits (e.g., clone's $self). This can drop the return value's refCount to 0 before the caller's LHS variables capture it, triggering DESTROY and clearing weak refs to still-live objects. The fix: - Adds MortalList.suppressFlush() to temporarily block flush() execution - RuntimeList.setFromList() suppresses flushing around the materialization and LHS assignment phase, then restores the previous state - Also adds reentrancy guard to flush() itself (try/finally with flushing flag) to prevent cascading DESTROY from re-entering flush() This fixes the DBIx::Class "detached result source" error where Schema->connect() returned an undefined value because the Schema clone was destroyed mid-construction. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Condensed P0 done section, corrected P1 hypothesis (callDestroy sets MIN_VALUE permanently, not a transient underflow), replaced speculative fix approaches with concrete debugging plan. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ised approach Step 11.2 (popMark + flush in setLargeRefCounted) was implemented and failed: mark-aware flush prevents DESTROY from firing for delete/untie/undef because subroutine calls push marks that hide earlier entries. 4 unit test regressions. Changes reverted. Added Step 11.3 with root cause analysis, comparison to Perl 5 FREETMPS model, and 4 possible approaches. Recommends Approach D (targeted GC leak fix) since P0 premature DESTROY is already solved by suppressFlush. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Blessed objects whose class has no DESTROY method (e.g., Moo objects like DBIx::Class::Storage::BlockRunner) were set to refCount=-1 (untracked) at bless time, so when they went out of scope their hash/array elements' refcounts were never decremented. Changes: - ReferenceOperators.bless(): always track all blessed objects regardless of whether DESTROY exists in the class hierarchy. Previously, classes without DESTROY got refCount=-1 (untracked). - DestroyDispatch.doCallDestroy(): when no DESTROY method is found, still cascade into hash/array elements via scopeExitCleanupHash/ scopeExitCleanupArray + flush() to decrement contained references. Test: dev/sandbox/destroy_weaken/destroy_no_destroy_method.t (13/13) All unit tests pass (make). Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…tails Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
- Step 11.4 fix committed and verified (all unit tests pass, 13/13 sandbox) - GC-only failures explained: Sub::Quote closure walk differences, not refcount bugs - Documented B::svref_2object->REFCNT method chain leak (separate bug) - Updated Next Steps and Open Questions with investigation results Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Complete handoff plan with 13 work items covering: - GC object liveness at END (146 files, 658 assertions) - DBI shim fixes (statement handles, transactions, numeric formatting, DBI_DRIVER, stringification, table locking, error handler) - Transaction/savepoint depth tracking - Detached ResultSource weak ref cleanup - B::svref_2object method chain refcount leak - UTF-8 byte-level string handling - Bless/overload performance Full suite baseline: 27 pass, 146 GC-only, 25 real fail, 43 skipped. 11,646 ok / 746 not-ok assertions. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Work Item 4: Added toJdbcValue() helper in DBI.java to convert
whole-number Doubles to Long before JDBC setObject(), fixing
10.0 vs 10 issue. Also handles overloaded object stringification.
Work Item 5: Fixed DBI.pm connect() to support empty driver in DSN,
$ENV{DBI_DRIVER} fallback, $ENV{DBI_DSN} fallback, proper error
messages, and require DBD::$driver for missing driver errors.
Work Item 6: Overloaded object stringification fixed by toJdbcValue().
Work Item 8: Added HandleError callback support in DBI.pm execute
wrapper, enabling DBIx::Class custom error handler.
Updated design doc with investigation findings for Work Item 2
(DBI statement handle finalization via cascading DESTROY).
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Avoid fork exhaustion by limiting parallel processes. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
do { BLOCK } return values were being prematurely destroyed when the
block contained lexical variables. The scope-exit flush processed
deferred decrements from inner subroutine returns before the caller
could capture the do-block result via assignment.
This fixes 11 of 12 "Unreachable cached statement still active" failures
in DBIx::Class t/60core.t. The Cursor DESTROY now fires at the correct
time, calling finish() on cached statement handles.
Root cause: do-blocks were treated as regular bare blocks (flush=true),
but like subroutine bodies, their return value is on the JVM operand
stack and must not be destroyed before the caller captures it.
Fix: Annotate do-blocks with blockIsDoBlock and skip mortal flush at
scope exit, matching the existing blockIsSubroutine behavior. Both
JVM backend (EmitBlock) and bytecode interpreter (BytecodeCompiler)
are updated.
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
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
1 participant
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
Post-merge fixes and re-baseline for DBIx::Class after DESTROY/weaken landed (PR #464).
Fixes (commit 756f9a4)
instanceofguards inBytecodeInterpreter.java— interpreter fallback registers can holdRuntimeScalarwhereRuntimeArraywas expectedtoArray()before iteratingDocumentation (commit 6586ec9)
dev/modules/dbix_class.mdwith Phase 9 re-baselineisweak()works, TxnScopeGuard fires, cached statement leak down from 12 to 1Test plan
makepasses./jcpan -t DBIx::Classruns without crashes (interpreter fallback and GlobalDestruction regressions fixed)Generated with Devin