core: prevent scope parent-chain cycles when reusing pooled frames
- Scope.resetForReuse: fully detach before re-parenting (clear state, parent=null, new frameId), then validate with ensureNoCycle and assign parent/args/pos/thisObj - ScopePool.borrow (JVM/Android/JS/Native/Wasm): defensive fallback to fresh Scope allocation if resetForReuse detects a cycle - docs: add docs/fix-scope-parent-cycle.md describing the change and expectations - test: add ScopeCycleRegressionTest to ensure instance method call pattern does not crash and returns "ok"
This commit is contained in:
parent
b953282251
commit
d15dfb6087
34
docs/fix-scope-parent-cycle.md
Normal file
34
docs/fix-scope-parent-cycle.md
Normal file
@ -0,0 +1,34 @@
|
||||
## Fix: prevent cycles in scope parent chain during pooled frame reuse
|
||||
|
||||
### What changed
|
||||
- Scope.resetForReuse now fully detaches a reused scope from its previous chain/state before re-parenting:
|
||||
- sets `parent = null` and regenerates `frameId`
|
||||
- clears locals/slots/bindings caches
|
||||
- only after that, validates the new parent with `ensureNoCycle` and assigns it
|
||||
- ScopePool.borrow on all targets (JVM, Android, JS, Native, Wasm) now has a defensive fallback:
|
||||
- if `resetForReuse` throws `IllegalStateException` indicating a parent-chain cycle, the pool allocates a fresh `Scope` instead of failing.
|
||||
|
||||
### Why
|
||||
In some nested call patterns (notably instance method calls where an instance is produced by another function and immediately used), the same pooled `Scope` object can be rebound into a chain that already (transitively) contains it. Reassigning `parent` in that case forms a structural cycle, which `ensureNoCycle` correctly detects and throws. This could surface as:
|
||||
|
||||
```
|
||||
IllegalStateException: cycle detected in scope parent chain assignment
|
||||
at net.sergeych.lyng.Scope.ensureNoCycle(...)
|
||||
at net.sergeych.lyng.Scope.resetForReuse(...)
|
||||
at net.sergeych.lyng.ScopePool.borrow(...)
|
||||
... during instance method invocation
|
||||
```
|
||||
|
||||
The fix removes the failure mode by:
|
||||
1) Detaching the reused frame from its prior chain/state before validating and assigning the new parent.
|
||||
2) Falling back to a new frame allocation if a cycle is still detected (extremely rare and cheap vs. a crash).
|
||||
|
||||
### Expected effects
|
||||
- Eliminates sporadic `cycle detected in scope parent chain` crashes during instance method invocation.
|
||||
- No change to public API or normal semantics.
|
||||
- Pooling remains enabled by default; the fallback only triggers on the detected cycle edge case.
|
||||
- Negligible performance impact: fresh allocation is used only when a cycle would have crashed the VM previously.
|
||||
|
||||
### Notes
|
||||
- The fix is platform-wide (all ScopePool actuals are covered).
|
||||
- We recommend adding/keeping a regression test that exercises: a class with a method, a function returning an instance, and an exported function calling the instance method. The test should pass without exceptions.
|
||||
@ -39,12 +39,18 @@ actual object ScopePool {
|
||||
actual fun borrow(parent: Scope, args: Arguments, pos: Pos, thisObj: Obj): Scope {
|
||||
val pool = pool()
|
||||
val s = if (pool.isNotEmpty()) pool.removeLast() else Scope(parent, args, pos, thisObj)
|
||||
if (s.parent !== parent || s.args !== args || s.pos !== pos || s.thisObj !== thisObj) {
|
||||
s.resetForReuse(parent, args, pos, thisObj)
|
||||
} else {
|
||||
s.frameId = nextFrameId()
|
||||
return try {
|
||||
if (s.parent !== parent || s.args !== args || s.pos !== pos || s.thisObj !== thisObj) {
|
||||
s.resetForReuse(parent, args, pos, thisObj)
|
||||
} else {
|
||||
s.frameId = nextFrameId()
|
||||
}
|
||||
s
|
||||
} catch (e: IllegalStateException) {
|
||||
if (e.message?.contains("cycle") == true && e.message?.contains("scope parent chain") == true) {
|
||||
Scope(parent, args, pos, thisObj)
|
||||
} else throw e
|
||||
}
|
||||
return s
|
||||
}
|
||||
|
||||
actual fun release(scope: Scope) {
|
||||
|
||||
@ -302,11 +302,9 @@ open class Scope(
|
||||
* Clears locals and slots, assigns new frameId, and sets parent/args/pos/thisObj.
|
||||
*/
|
||||
fun resetForReuse(parent: Scope?, args: Arguments, pos: Pos, thisObj: Obj) {
|
||||
ensureNoCycle(parent)
|
||||
this.parent = parent
|
||||
this.args = args
|
||||
this.pos = pos
|
||||
this.thisObj = thisObj
|
||||
// Fully detach from any previous chain/state first to avoid residual ancestry
|
||||
// that could interact badly with the new parent and produce a cycle.
|
||||
this.parent = null
|
||||
this.skipScopeCreation = false
|
||||
// fresh identity for PIC caches
|
||||
this.frameId = nextFrameId()
|
||||
@ -315,6 +313,12 @@ open class Scope(
|
||||
slots.clear()
|
||||
nameToSlot.clear()
|
||||
localBindings.clear()
|
||||
// Now safe to validate and re-parent
|
||||
ensureNoCycle(parent)
|
||||
this.parent = parent
|
||||
this.args = args
|
||||
this.pos = pos
|
||||
this.thisObj = thisObj
|
||||
// Pre-size local slots for upcoming parameter assignment where possible
|
||||
reserveLocalCapacity(args.list.size + 4)
|
||||
}
|
||||
|
||||
34
lynglib/src/commonTest/kotlin/ScopeCycleRegressionTest.kt
Normal file
34
lynglib/src/commonTest/kotlin/ScopeCycleRegressionTest.kt
Normal file
@ -0,0 +1,34 @@
|
||||
/*
|
||||
* Regression test for scope parent-chain cycle when reusing pooled frames
|
||||
*/
|
||||
|
||||
import kotlinx.coroutines.test.runTest
|
||||
import net.sergeych.lyng.eval
|
||||
import kotlin.test.Test
|
||||
|
||||
class ScopeCycleRegressionTest {
|
||||
@Test
|
||||
fun instanceMethodCallDoesNotCycle() = runTest {
|
||||
eval(
|
||||
(
|
||||
"""
|
||||
class Whatever {
|
||||
fun something() {
|
||||
trace("something")
|
||||
}
|
||||
}
|
||||
|
||||
fun ll() { Whatever() }
|
||||
|
||||
fun callTest1() {
|
||||
val l = ll()
|
||||
l.something()
|
||||
"ok"
|
||||
}
|
||||
|
||||
assertEquals("ok", callTest1())
|
||||
"""
|
||||
).trimIndent()
|
||||
)
|
||||
}
|
||||
}
|
||||
@ -29,12 +29,18 @@ actual object ScopePool {
|
||||
|
||||
actual fun borrow(parent: Scope, args: Arguments, pos: Pos, thisObj: Obj): Scope {
|
||||
val s = if (pool.isNotEmpty()) pool.removeLast() else Scope(parent, args, pos, thisObj)
|
||||
if (s.parent !== parent || s.args !== args || s.pos !== pos || s.thisObj !== thisObj) {
|
||||
s.resetForReuse(parent, args, pos, thisObj)
|
||||
} else {
|
||||
s.frameId = nextFrameId()
|
||||
return try {
|
||||
if (s.parent !== parent || s.args !== args || s.pos !== pos || s.thisObj !== thisObj) {
|
||||
s.resetForReuse(parent, args, pos, thisObj)
|
||||
} else {
|
||||
s.frameId = nextFrameId()
|
||||
}
|
||||
s
|
||||
} catch (e: IllegalStateException) {
|
||||
if (e.message?.contains("cycle") == true && e.message?.contains("scope parent chain") == true) {
|
||||
Scope(parent, args, pos, thisObj)
|
||||
} else throw e
|
||||
}
|
||||
return s
|
||||
}
|
||||
|
||||
actual fun release(scope: Scope) {
|
||||
|
||||
@ -33,9 +33,19 @@ actual object ScopePool {
|
||||
actual fun borrow(parent: Scope, args: Arguments, pos: Pos, thisObj: Obj): Scope {
|
||||
val pool = threadLocalPool.get()
|
||||
val s = if (pool.isNotEmpty()) pool.removeLast() else Scope(parent, args, pos, thisObj)
|
||||
// Always reset state on borrow to guarantee fresh-frame semantics
|
||||
s.resetForReuse(parent, args, pos, thisObj)
|
||||
return s
|
||||
return try {
|
||||
// Always reset state on borrow to guarantee fresh-frame semantics
|
||||
s.resetForReuse(parent, args, pos, thisObj)
|
||||
s
|
||||
} catch (e: IllegalStateException) {
|
||||
// Defensive fallback: if a cycle in scope parent chain is detected during reuse,
|
||||
// discard pooled instance for this call frame and allocate a fresh scope instead.
|
||||
if (e.message?.contains("cycle") == true && e.message?.contains("scope parent chain") == true) {
|
||||
Scope(parent, args, pos, thisObj)
|
||||
} else {
|
||||
throw e
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
actual fun release(scope: Scope) {
|
||||
|
||||
@ -29,12 +29,18 @@ actual object ScopePool {
|
||||
|
||||
actual fun borrow(parent: Scope, args: Arguments, pos: Pos, thisObj: Obj): Scope {
|
||||
val s = if (pool.isNotEmpty()) pool.removeLast() else Scope(parent, args, pos, thisObj)
|
||||
if (s.parent !== parent || s.args !== args || s.pos !== pos || s.thisObj !== thisObj) {
|
||||
s.resetForReuse(parent, args, pos, thisObj)
|
||||
} else {
|
||||
s.frameId = nextFrameId()
|
||||
return try {
|
||||
if (s.parent !== parent || s.args !== args || s.pos !== pos || s.thisObj !== thisObj) {
|
||||
s.resetForReuse(parent, args, pos, thisObj)
|
||||
} else {
|
||||
s.frameId = nextFrameId()
|
||||
}
|
||||
s
|
||||
} catch (e: IllegalStateException) {
|
||||
if (e.message?.contains("cycle") == true && e.message?.contains("scope parent chain") == true) {
|
||||
Scope(parent, args, pos, thisObj)
|
||||
} else throw e
|
||||
}
|
||||
return s
|
||||
}
|
||||
|
||||
actual fun release(scope: Scope) {
|
||||
|
||||
@ -29,12 +29,18 @@ actual object ScopePool {
|
||||
|
||||
actual fun borrow(parent: Scope, args: Arguments, pos: Pos, thisObj: Obj): Scope {
|
||||
val s = if (pool.isNotEmpty()) pool.removeLast() else Scope(parent, args, pos, thisObj)
|
||||
if (s.parent !== parent || s.args !== args || s.pos !== pos || s.thisObj !== thisObj) {
|
||||
s.resetForReuse(parent, args, pos, thisObj)
|
||||
} else {
|
||||
s.frameId = nextFrameId()
|
||||
return try {
|
||||
if (s.parent !== parent || s.args !== args || s.pos !== pos || s.thisObj !== thisObj) {
|
||||
s.resetForReuse(parent, args, pos, thisObj)
|
||||
} else {
|
||||
s.frameId = nextFrameId()
|
||||
}
|
||||
s
|
||||
} catch (e: IllegalStateException) {
|
||||
if (e.message?.contains("cycle") == true && e.message?.contains("scope parent chain") == true) {
|
||||
Scope(parent, args, pos, thisObj)
|
||||
} else throw e
|
||||
}
|
||||
return s
|
||||
}
|
||||
|
||||
actual fun release(scope: Scope) {
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user