Skip to content

Register one script change listener per script instead of per error - #71

Closed
rubensworks wants to merge 1 commit into
master-26-ltsfrom
claude/issue-67-error-listener-leak
Closed

Register one script change listener per script instead of per error#71
rubensworks wants to merge 1 commit into
master-26-ltsfrom
claude/issue-67-error-listener-leak

Conversation

@rubensworks

Copy link
Copy Markdown
Member

Fixes the second half of #67: a script that keeps failing accumulates script change listeners without bound, and cleaning them up is quadratic. This is what makes the reporter's unguarded filter degrade over time rather than just being slow.

Problem

resolveOnScriptChange registers a script change listener for every created EvaluationException, so it can be resolved when its script changes. Those listeners are only removed once the exception has been collected AND expungeStaleEvaluationExceptions() runs, and that only happens when a new exception factory is created, i.e. in GraalScript#getValue. A ScriptVariable caches its value, so getValue is not called again while the script stays valid, and the exceptions created by every failing evaluation pile up in ScriptingData's listener list.

Removal is quadratic on top of that: each listener is removed separately from an ArrayList, so expunging k of n listeners is O(n·k), in one tick.

Measured with a throwaway test on master-26-lts:

listeners before: 0
listeners after 10000 errors: 10000
listeners after gc: 10000
listeners after expunge: 0 (expunge took 28.151372ms)

Fix

Exceptions are now grouped per script behind a single ScriptExceptionsListener, which:

  • resolves all its exceptions when its script changes, and unregisters itself,
  • holds them as weak references in a set, so expunging a collected one is a hash removal instead of a list scan, and unregisters itself once its last exception is gone.

resolveOnScriptChange also expunges first, so a repeatedly failing script no longer accumulates exceptions between evaluations.

These listeners belong to one ScriptingData instance, so they are forgotten in onServerStopping alongside it. Without that, listeners registered before a server stop would keep the old keys around and never be registered on the next server's scripting data.

Tests

EvaluationExceptionResolutionHelpersTest covers the listener count for many exceptions, one listener per script rather than per exception, resolution on script change (and only for the matching script), and listener removal once the exceptions are collected. Verified that the same test file fails on master-26-lts without the fix:

EvaluationExceptionResolutionHelpersTest > testSingleListenerForManyExceptions() FAILED
    Expected: is <1>
         but: was <1000>
EvaluationExceptionResolutionHelpersTest > testListenersAreRemovedAfterExceptionsAreCollected() FAILED
    Expected: is <1>
         but: was <100>
EvaluationExceptionResolutionHelpersTest > testListenerPerScript() FAILED
    Expected: is <3>
         but: was <6>
4 tests completed, 3 failed

testExceptionsAreResolvedOnScriptChange passes both before and after, as the behaviour it checks is unchanged.

./gradlew spotlessApply build passes with the fix. I could not run runGameTestServer in this environment.

Note

This removes the leak, not the cost of the exception itself. An exception escaping a script measures around 0.45ms in a benchmark against 0.018ms for the same script without one, and the cost is the guest-to-host boundary: the same JS error caught inside JS is 0.010ms, and reading getMessage() adds nothing measurable. Scripts in hot paths should still avoid throwing.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JCW1HmWhLT27jh57t9d7B6


Generated by Claude Code

Every created script error registered its own script change listener, to
resolve that error when the script changes. Those listeners were only
removed once their exception was collected AND a new exception factory was
created, which does not happen while a script variable holds on to its
value, so a script that keeps failing accumulated a listener per failed
evaluation. Removing them afterwards was quadratic on top of that, because
each one was removed separately from a list: 10000 errors left 10000
listeners behind, which took 28ms to expunge.

Exceptions are now collected per script, behind a single listener that
resolves all of them when that script changes, and drops them one by one
as they are collected. Registering a new exception expunges the collected
ones first, so failing scripts no longer accumulate them at all.

These listeners belong to the scripting data they were registered on, so
they are also forgotten when the server stops.

Related to #67

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JCW1HmWhLT27jh57t9d7B6
@coveralls

Copy link
Copy Markdown

Coverage Report for CI Build 33628141471

Coverage increased (+1.4%) to 50.984%

Details

  • Coverage increased (+1.4%) from the base build.
  • Patch coverage: No coverable lines changed in this PR.
  • 4 coverage regressions across 2 files.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

4 previously-covered lines in 2 files lost coverage.

File Lines Losing Coverage Coverage
org/cyclops/integratedscripting/IntegratedScripting.java 3 89.71%
org/cyclops/integratedscripting/evaluate/EvaluationExceptionResolutionHelpers.java 1 92.31%

Coverage Stats

Coverage Status
Relevant Lines: 2598
Covered Lines: 1453
Line Coverage: 55.93%
Relevant Branches: 856
Covered Branches: 308
Branch Coverage: 35.98%
Branches in Coverage %: Yes
Coverage Strength: 20.18 hits per line

💛 - Coveralls

Copy link
Copy Markdown
Member Author

Superseded by #74, which carries the same change against master-1.20-lts so it can be upmerged. EvaluationExceptionResolutionHelpers is identical on 1.20, 1.21 and 26, so nothing is lost by taking it there instead. Closing this one.


Generated by Claude Code

@rubensworks rubensworks closed this Sep 2, 2026
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.

3 participants