diff --git a/dev/modules/role_basic.md b/dev/modules/role_basic.md index f686f86d6..0e144b157 100644 --- a/dev/modules/role_basic.md +++ b/dev/modules/role_basic.md @@ -1,6 +1,6 @@ # Role::Basic (CPAN) on PerlOnJava -Documentation-only deliverable: this file is the **plan** (symptom, bisection, hypothesized fix, verification). Landing the **`RuntimeCode` `EvalRuntimeContext` stack** (or another confirmed fix) is a **follow-up** change once `timeout 600 ./jcpan -t Role::Basic` passes. +Implemented deliverable: `Role::Basic` passes under PerlOnJava after fixing nested eval runtime context handling in `RuntimeCode`. ## Acceptance @@ -23,7 +23,7 @@ Upstream logic (simplified): after `eval "use $role $version"`, code does `retur - Bisection on a forked `Role/Basic.pm`: removing the stash preamble (`my $stash = do { no strict 'refs'; \%{"${role}::"} };` and related `%INC` logic) made jperl match stock perl — implicating **interaction** of that block with **nested compilation**, not `"\%{"${role}::"}"` vs `$role . '::'` alone (a minimal two-level reentrancy test with only interpolation can still pass). - Copying stash keys in `HashSpecialVariable.getStash` / `GlobalVariable.getGlobalHash` did **not** fix jcpan; treat as unrelated unless a future test proves otherwise. -## Root cause (target fix) +## Root Cause And Fix **Nested `eval STRING` compilation** uses a single `ThreadLocal` for `EvalRuntimeContext` in [`RuntimeCode.java`](../../src/main/java/org/perlonjava/runtime/runtimetypes/RuntimeCode.java): inner `evalStringHelper` / `evalStringWithInterpreter` **overwrites** the context and the outer `finally` block calls **`remove()`**, dropping the outer eval’s context while outer compilation may still run (e.g. `use` during compile → nested `_load_role` → nested `eval`). That matches the BEGIN-alias cleanup comment in the same file about **recursive** eval corrupting globals. @@ -36,6 +36,8 @@ Upstream logic (simplified): after `eval "use $role $version"`, code does `retur Apply consistently in both `evalStringHelper` and `evalStringWithInterpreter`. +The passing implementation also tracks the BEGIN-package aliases installed for eval parsing and deactivates them when `saveAndClearEvalRuntimeContext()` is used around `require` / `do` compilation. The stack alone preserves the runtime context, but Role::Basic also needed those temporary aliases hidden while a `use $role` compile re-enters `_load_role`; otherwise the inner call retrieves and mutates the outer eval's `$role` scalar. + ## Neutral regression tests - Prefer a small `.t` under `src/test/resources/unit/` that does **not** bundle Role::Basic: nested `eval` + `do { no strict 'refs'; \%{"${pkg}::"} }` + outer lexical preservation. If no stable minimal repro, rely on jcpan Role::Basic plus existing patterns like `stash_lexical_reentrancy.t` / `sub_reentrant_lexical_register.t` as guards. @@ -54,8 +56,27 @@ Apply consistently in both `evalStringHelper` and `evalStringWithInterpreter`. 4. Branch from `master`, commit with [AI_POLICY.md](../AI_POLICY.md) attribution (`git commit -F file`). 5. Push and `gh pr create --body-file /tmp/pr_body.md` (never `--body` with inline backticks). -## Progress +## Progress Tracking + +### Current Status: Completed 2026-05-08 + +### Completed Work + +- [x] Implemented eval runtime context stack in [`RuntimeCode.java`](../../src/main/java/org/perlonjava/runtime/runtimetypes/RuntimeCode.java) +- [x] Tracked eval BEGIN aliases so they can be temporarily hidden around nested module compilation +- [x] Added neutral regression test [`eval_context_stack_reentrancy.t`](../../src/test/resources/unit/eval_context_stack_reentrancy.t) + +### Verification + +- `make` -> pass +- `timeout 60 ./jperl src/test/resources/unit/eval_context_stack_reentrancy.t` -> pass +- `timeout 600 ./jcpan -t Role::Basic` -> pass, 16 files / 304 tests + +### Next Steps + +1. Commit the fix on `fix/role-basic-eval-context-stack`. +2. Open a PR with the verification above. + +### Open Questions -| Date | Status | -|------------|--------| -| 2026-05-08 | Plan documented in-repo (`role_basic.md` only). Runtime fix pending (jcpan still fails `t/exceptions.t` test 1 until verified). | +- None. diff --git a/src/main/java/org/perlonjava/app/scriptengine/PerlLanguageProvider.java b/src/main/java/org/perlonjava/app/scriptengine/PerlLanguageProvider.java index f9b66c316..3f806539f 100644 --- a/src/main/java/org/perlonjava/app/scriptengine/PerlLanguageProvider.java +++ b/src/main/java/org/perlonjava/app/scriptengine/PerlLanguageProvider.java @@ -96,7 +96,7 @@ public static RuntimeList executePerlCode(CompilerOptions compilerOptions, // local variables in required modules to the eval's captured variables when // they share the same name (e.g., $caller in constant.pm vs $caller in eval scope). RuntimeCode.EvalRuntimeContext savedEvalRuntimeContext = - RuntimeCode.saveAndClearEvalRuntimeContext(); + RuntimeCode.saveAndClearEvalRuntimeContextAndAliases(); // Store the isMainProgram flag in CompilerOptions for use during code generation compilerOptions.isMainProgram = isTopLevelScript; diff --git a/src/main/java/org/perlonjava/runtime/runtimetypes/RuntimeCode.java b/src/main/java/org/perlonjava/runtime/runtimetypes/RuntimeCode.java index f46275ba9..63605a154 100644 --- a/src/main/java/org/perlonjava/runtime/runtimetypes/RuntimeCode.java +++ b/src/main/java/org/perlonjava/runtime/runtimetypes/RuntimeCode.java @@ -78,7 +78,7 @@ public class RuntimeCode extends RuntimeBase implements RuntimeScalarReference { public static final boolean EVAL_TRACE = System.getenv("JPERL_EVAL_TRACE") != null; /** - * ThreadLocal storage for runtime values of captured variables during eval STRING compilation. + * ThreadLocal stack for runtime values of captured variables during eval STRING compilation. *

* PROBLEM: In perl5, BEGIN blocks inside eval STRING can access outer lexical variables' runtime values: * my @imports = qw(a b); @@ -87,7 +87,7 @@ public class RuntimeCode extends RuntimeBase implements RuntimeScalarReference { * In PerlOnJava, BEGIN blocks execute during parsing (before the eval class is instantiated), * so they couldn't access runtime values - they would see empty variables. *

- * SOLUTION: When evalStringHelper() is called, the runtime values are stored in this ThreadLocal. + * SOLUTION: When evalStringHelper() is called, the runtime values are pushed onto this ThreadLocal stack. * During parsing, when SpecialBlockParser sets up BEGIN blocks, it can access these runtime values * and use them to initialize the special globals that lexical variables become in BEGIN blocks. *

@@ -97,10 +97,12 @@ public class RuntimeCode extends RuntimeBase implements RuntimeScalarReference { * - runtimeValues: Object[] of captured variable values * - capturedEnv: String[] of captured variable names (matching array indices) *

- * Thread-safety: Each thread's eval compilation uses its own ThreadLocal storage, so parallel - * eval compilations don't interfere with each other. + * Thread-safety: Each thread's eval compilation uses its own ThreadLocal stack, so parallel + * eval compilations don't interfere with each other. A stack is required because eval STRING + * compilation can re-enter eval STRING compilation via BEGIN/use/require. */ - private static final ThreadLocal evalRuntimeContext = new ThreadLocal<>(); + private static final ThreadLocal> evalRuntimeContextStack = + ThreadLocal.withInitial(ArrayDeque::new); // Cache for memoization of evalStringHelper results private static final int CLASS_CACHE_SIZE = 100; private static final Map> evalCache = new LinkedHashMap>(CLASS_CACHE_SIZE, 0.75f, true) { @@ -680,7 +682,8 @@ public static boolean hasAutoload(RuntimeCode code) { * @return The current eval runtime context, or null if not in eval STRING compilation */ public static EvalRuntimeContext getEvalRuntimeContext() { - return evalRuntimeContext.get(); + ArrayDeque stack = evalRuntimeContextStack.get(); + return stack.isEmpty() ? null : stack.peekFirst(); } /** @@ -691,9 +694,21 @@ public static EvalRuntimeContext getEvalRuntimeContext() { * @return The saved eval runtime context (may be null) */ public static EvalRuntimeContext saveAndClearEvalRuntimeContext() { - EvalRuntimeContext saved = evalRuntimeContext.get(); + ArrayDeque stack = evalRuntimeContextStack.get(); + if (stack.isEmpty()) { + return null; + } + EvalRuntimeContext saved = stack.removeFirst(); + if (stack.isEmpty()) { + evalRuntimeContextStack.remove(); + } + return saved; + } + + public static EvalRuntimeContext saveAndClearEvalRuntimeContextAndAliases() { + EvalRuntimeContext saved = saveAndClearEvalRuntimeContext(); if (saved != null) { - evalRuntimeContext.remove(); + deactivateEvalRuntimeAliases(saved); } return saved; } @@ -705,7 +720,79 @@ public static EvalRuntimeContext saveAndClearEvalRuntimeContext() { */ public static void restoreEvalRuntimeContext(EvalRuntimeContext saved) { if (saved != null) { - evalRuntimeContext.set(saved); + evalRuntimeContextStack.get().addFirst(saved); + reactivateEvalRuntimeAliases(saved); + } + } + + private static void pushEvalRuntimeContext(EvalRuntimeContext context) { + evalRuntimeContextStack.get().addFirst(context); + } + + private static void popEvalRuntimeContext(EvalRuntimeContext context) { + ArrayDeque stack = evalRuntimeContextStack.get(); + if (!stack.isEmpty()) { + if (stack.peekFirst() == context) { + stack.removeFirst(); + } else { + for (Iterator it = stack.iterator(); it.hasNext(); ) { + if (it.next() == context) { + it.remove(); + break; + } + } + } + } + if (stack.isEmpty()) { + evalRuntimeContextStack.remove(); + } + } + + private static void registerEvalRuntimeAlias(EvalRuntimeContext context, char sigil, String fullName, RuntimeBase value) { + context.aliases().add(new EvalRuntimeAlias(sigil, fullName, value)); + } + + private static void deactivateEvalRuntimeAliases(EvalRuntimeContext context) { + for (EvalRuntimeAlias alias : context.aliases()) { + if (!alias.active) { + continue; + } + switch (alias.sigil) { + case '$' -> { + if (GlobalVariable.globalVariables.get(alias.fullName) == alias.value) { + GlobalVariable.globalVariables.remove(alias.fullName); + } + } + case '@' -> { + if (GlobalVariable.globalArrays.get(alias.fullName) == alias.value) { + GlobalVariable.globalArrays.remove(alias.fullName); + } + } + case '%' -> { + if (GlobalVariable.globalHashes.get(alias.fullName) == alias.value) { + GlobalVariable.globalHashes.remove(alias.fullName); + } + } + } + alias.active = false; + } + } + + private static void reactivateEvalRuntimeAliases(EvalRuntimeContext context) { + for (EvalRuntimeAlias alias : context.aliases()) { + if (alias.active) { + continue; + } + boolean installed = switch (alias.sigil) { + case '$' -> alias.value instanceof RuntimeScalar scalar + && GlobalVariable.globalVariables.putIfAbsent(alias.fullName, scalar) == null; + case '@' -> alias.value instanceof RuntimeArray array + && GlobalVariable.globalArrays.putIfAbsent(alias.fullName, array) == null; + case '%' -> alias.value instanceof RuntimeHash hash + && GlobalVariable.globalHashes.putIfAbsent(alias.fullName, hash) == null; + default -> false; + }; + alias.active = installed; } } @@ -726,7 +813,7 @@ public static void clearCaches() { anonSubs.clear(); interpretedSubs.clear(); evalContext.clear(); - evalRuntimeContext.remove(); + evalRuntimeContextStack.remove(); } public static void copy(RuntimeCode code, RuntimeCode codeFrom) { @@ -814,7 +901,7 @@ public static Class evalStringHelper(RuntimeScalar code, String evalTag, Obje ctx.capturedEnv, // Variable names in same order as runtimeValues evalTag ); - evalRuntimeContext.set(runtimeCtx); + pushEvalRuntimeContext(runtimeCtx); try { // Check if the eval string contains non-ASCII characters @@ -925,7 +1012,6 @@ public static Class evalStringHelper(RuntimeScalar code, String evalTag, Obje // We create: globalArrays["BEGIN_PKG_x::@arr"] = (the runtime @arr object) // Then when "say @arr" is parsed in the BEGIN, it resolves to BEGIN_PKG_x::@arr // which is aliased to the runtime array with values (a, b). - List evalAliasKeys = new ArrayList<>(); Map capturedVars = capturedSymbolTable.getAllVisibleVariables(); for (SymbolTable.SymbolEntry entry : capturedVars.values()) { if (!entry.name().equals("@_") && !entry.decl().isEmpty() && !entry.name().startsWith("&")) { @@ -964,7 +1050,7 @@ public static Class evalStringHelper(RuntimeScalar code, String evalTag, Obje } } if (installed) { - evalAliasKeys.add(entry.name().charAt(0) + fullName); + registerEvalRuntimeAlias(runtimeCtx, entry.name().charAt(0), fullName, (RuntimeBase) runtimeValue); } } } @@ -1103,14 +1189,8 @@ public static Class evalStringHelper(RuntimeScalar code, String evalTag, Obje // if a recursive call re-enters the same function and its `my` declaration // calls retrieveBeginScalar, finding the stale alias instead of creating // a fresh variable. - for (String key : evalAliasKeys) { - String fullName = key.substring(1); - switch (key.charAt(0)) { - case '$' -> GlobalVariable.globalVariables.remove(fullName); - case '@' -> GlobalVariable.globalArrays.remove(fullName); - case '%' -> GlobalVariable.globalHashes.remove(fullName); - } - } + deactivateEvalRuntimeAliases(runtimeCtx); + runtimeCtx.aliases().clear(); // Store source lines in symbol table if $^P flags are set // Do this on both success and failure paths when flags require retention @@ -1132,11 +1212,9 @@ public static Class evalStringHelper(RuntimeScalar code, String evalTag, Obje // This MUST be in the outer finally to handle both cache hits and compilation paths. setCurrentScope(savedCurrentScope); - // Clean up ThreadLocal to prevent memory leaks - // IMPORTANT: Always clean up ThreadLocal in finally block to ensure it's removed - // even if compilation fails. Failure to do so could cause memory leaks in - // long-running applications with thread pools. - evalRuntimeContext.remove(); + // Clean up this eval's ThreadLocal stack entry to prevent memory leaks. + // IMPORTANT: Always pop in the finally block even if compilation fails. + popEvalRuntimeContext(runtimeCtx); } } @@ -1302,7 +1380,7 @@ public static RuntimeList evalStringWithInterpreter( ctx.capturedEnv, evalTag ); - evalRuntimeContext.set(runtimeCtx); + pushEvalRuntimeContext(runtimeCtx); InterpretedCode interpretedCode = null; RuntimeList result; @@ -1310,7 +1388,6 @@ public static RuntimeList evalStringWithInterpreter( // Declare these outside try block so they're accessible in finally block for debugger support Node ast = null; List tokens = null; - List evalAliasKeys = new ArrayList<>(); // Save dynamic variable level to restore after eval. // IMPORTANT: Scope InterpreterState.currentPackage around eval execution. @@ -1401,7 +1478,7 @@ public static RuntimeList evalStringWithInterpreter( } } if (installed) { - evalAliasKeys.add(entry.name().charAt(0) + fullName); + registerEvalRuntimeAlias(runtimeCtx, entry.name().charAt(0), fullName, (RuntimeBase) runtimeValue); } } } @@ -1586,15 +1663,8 @@ public static RuntimeList evalStringWithInterpreter( // GlobalVariable during execution, a recursive call that re-enters the same // function would find the alias via retrieveBeginScalar, sharing the same // RuntimeScalar object instead of creating a fresh one. - for (String key : evalAliasKeys) { - String fullName = key.substring(1); - switch (key.charAt(0)) { - case '$' -> GlobalVariable.globalVariables.remove(fullName); - case '@' -> GlobalVariable.globalArrays.remove(fullName); - case '%' -> GlobalVariable.globalHashes.remove(fullName); - } - } - evalAliasKeys.clear(); + deactivateEvalRuntimeAliases(runtimeCtx); + runtimeCtx.aliases().clear(); // Execute the interpreted code // Track eval depth for $^S support @@ -1678,8 +1748,8 @@ public static RuntimeList evalStringWithInterpreter( storeSourceLines(code.toString(), evalFilename, ast, tokens); } - // Clean up ThreadLocal - evalRuntimeContext.remove(); + // Clean up this eval's ThreadLocal stack entry. + popEvalRuntimeContext(runtimeCtx); } } @@ -3939,34 +4009,55 @@ public void dynamicRestoreState() { } /** - * Container for runtime context during eval STRING compilation. - * Holds both the runtime values and variable names so SpecialBlockParser can - * match variables to their values. - */ - public record EvalRuntimeContext(Object[] runtimeValues, String[] capturedEnv, String evalTag) { + * Tracks one temporary BEGIN-package alias installed for eval STRING parsing. + */ + public static final class EvalRuntimeAlias { + final char sigil; + final String fullName; + final RuntimeBase value; + boolean active = true; + + EvalRuntimeAlias(char sigil, String fullName, RuntimeBase value) { + this.sigil = sigil; + this.fullName = fullName; + this.value = value; + } + } + + /** + * Container for runtime context during eval STRING compilation. + * Holds both the runtime values and variable names so SpecialBlockParser can + * match variables to their values. + */ + public record EvalRuntimeContext(Object[] runtimeValues, String[] capturedEnv, String evalTag, + List aliases) { + + public EvalRuntimeContext(Object[] runtimeValues, String[] capturedEnv, String evalTag) { + this(runtimeValues, capturedEnv, evalTag, new ArrayList<>()); + } /** - * Get the runtime value for a variable by name. - *

- * IMPORTANT: The capturedEnv array includes all variables (including 'this', '@_', 'wantarray'), - * but runtimeValues array skips the first skipVariables (currently 3). - * So if @imports is at capturedEnv[5], its value is at runtimeValues[5-3=2]. - * - * @param varName The variable name (e.g., "@imports", "$scalar") - * @return The runtime value, or null if not found - */ - public Object getRuntimeValue(String varName) { - int skipVariables = 3; // 'this', '@_', 'wantarray' - for (int i = skipVariables; i < capturedEnv.length; i++) { - if (varName.equals(capturedEnv[i])) { - int runtimeIndex = i - skipVariables; - if (runtimeIndex >= 0 && runtimeIndex < runtimeValues.length) { - return runtimeValues[runtimeIndex]; - } + * Get the runtime value for a variable by name. + *

+ * IMPORTANT: The capturedEnv array includes all variables (including 'this', '@_', 'wantarray'), + * but runtimeValues array skips the first skipVariables (currently 3). + * So if @imports is at capturedEnv[5], its value is at runtimeValues[5-3=2]. + * + * @param varName The variable name (e.g., "@imports", "$scalar") + * @return The runtime value, or null if not found + */ + public Object getRuntimeValue(String varName) { + int skipVariables = 3; // 'this', '@_', 'wantarray' + for (int i = skipVariables; i < capturedEnv.length; i++) { + if (varName.equals(capturedEnv[i])) { + int runtimeIndex = i - skipVariables; + if (runtimeIndex >= 0 && runtimeIndex < runtimeValues.length) { + return runtimeValues[runtimeIndex]; } } - return null; } + return null; } + } } diff --git a/src/test/resources/unit/eval_context_stack_reentrancy.t b/src/test/resources/unit/eval_context_stack_reentrancy.t new file mode 100644 index 000000000..ba96f6f0a --- /dev/null +++ b/src/test/resources/unit/eval_context_stack_reentrancy.t @@ -0,0 +1,133 @@ +use strict; +use warnings; +use Test::More tests => 3; +use File::Path qw(make_path); + +BEGIN { + $INC{'EvalContextStackRepro/Registry.pm'} = __FILE__; +} + +package EvalContextStackRepro::Registry; + +use strict; +use warnings; +use Carp (); + +my %IS_ROLE; +our ( $AFTER_EVAL_ROLE, $EARLY_RETURN_ROLE ); + +sub import { + my $class = shift; + my $target = caller; + + if ( @_ == 1 && $_[0] eq 'with' ) { + no strict 'refs'; + *{ $target . '::with' } = sub { + $class->apply_roles_to_package( $target, @_ ); + }; + return; + } + + $class->_declare_role($target); +} + +sub _declare_role { + my ( $class, $target ) = @_; + $IS_ROLE{$target} = 1; + no strict 'refs'; + *{ $target . '::requires' } = sub { return }; +} + +sub apply_roles_to_package { + my ( $class, $target, @roles ) = @_; + $class->_load_role($_) for @roles; +} + +sub _load_role { + my ( $class, $role, $version ) = @_; + + $version ||= ''; + my $stash = do { no strict 'refs'; \%{"${role}::"} }; + if ( exists $stash->{requires} ) { + my $package = $role; + $package =~ s{::}{/}g; + $package .= '.pm'; + $INC{$package} ||= "added to inc by $class"; + } + + eval "use $role $version"; + Carp::confess($@) if $@; + + $AFTER_EVAL_ROLE = $role; + if ( $IS_ROLE{$role} ) { + $EARLY_RETURN_ROLE = $role; + return 1; + } + + my $requires = $role->can('requires'); + if ( !$requires ) { + Carp::confess( + "Only roles defined with $class may be loaded with _load_role. '$role' is not allowed." + ); + } + + $IS_ROLE{$role} = 1; + return 1; +} + +package main; + +sub write_file { + my ( $path, $body ) = @_; + open my $fh, '>', $path or die "open $path: $!"; + print {$fh} $body or die "write $path: $!"; + close $fh or die "close $path: $!"; +} + +my $root = "/tmp/perlonjava-eval-context-stack-$$"; +my $lib = "$root/lib"; +make_path("$lib/EvalContextStackRepro"); +unshift @INC, $lib; + +write_file( + "$lib/EvalContextStackRepro/Plugin.pm", + <<'PLUGIN' +package EvalContextStackRepro::Plugin; +use EvalContextStackRepro::Registry; +1; +PLUGIN +); + +write_file( + "$lib/EvalContextStackRepro/Host.pm", + <<'HOST' +package EvalContextStackRepro::Host; +use EvalContextStackRepro::Registry 'with'; +with 'EvalContextStackRepro::Plugin'; +sub required_method {} +1; +HOST +); + +eval { EvalContextStackRepro::Registry->_load_role('EvalContextStackRepro::Plugin') }; +ok( !$@, 'loading a marked package succeeds' ) or diag $@; + +eval { EvalContextStackRepro::Registry->_load_role('EvalContextStackRepro::Host') }; +like( + $@, + qr/Only roles defined with EvalContextStackRepro::Registry may be loaded/, + 'outer eval role name survives nested eval use' +) or diag( + "after eval role=[$EvalContextStackRepro::Registry::AFTER_EVAL_ROLE], " + . "early return role=[$EvalContextStackRepro::Registry::EARLY_RETURN_ROLE]" +); + +my @begin_seen; +eval q{ + BEGIN { @begin_seen = map { $_ => eval { die } || -1 } qw(ABC XYZ); } +}; +is( + "@begin_seen", + "ABC -1 XYZ -1", + 'BEGIN execution keeps eval lexical aliases visible' +) or diag $@;