Groovy class loaders and script engine tweaks to reduce leaks#857
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughScript factories now expose destruction hooks and close Groovy classloaders. Site context shutdown and creation-failure paths invoke these hooks, while context rebuilds handle destruction failures through the manager. ChangesResource lifecycle management
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant SiteContextFactory
participant SiteContext
participant ScriptFactory
participant StoreService
SiteContextFactory->>SiteContext: initialize and wire context resources
SiteContextFactory->>SiteContext: destroy partially created context on failure
SiteContext->>ScriptFactory: destroy script factory
SiteContextFactory->>StoreService: destroy store context
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/org/craftercms/engine/service/context/SiteContext.java (1)
626-633: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFormat the inline comment and consistently null out references.
Consider moving the inline comment to its own line for better readability. Additionally, it is a good practice to clear the
applicationContextreference in afinallyblock to prevent lingering references if theSiteContextobject stays in memory longer than expected, keeping it consistent with the handling ofscriptFactoryandclassLoaderbelow it.♻️ Proposed refactor
if (applicationContext != null) { try { applicationContext.close(); } catch (Exception e) { logger.error("Error while closing application context for {}", this, e); - } - } // Close the GroovyScriptEngine's ScriptClassLoader (child) before the parent site class loader + } finally { + applicationContext = null; + } + } + + // Close the GroovyScriptEngine's ScriptClassLoader (child) before the parent site class loader if (scriptFactory != null) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/craftercms/engine/service/context/SiteContext.java` around lines 626 - 633, Move the GroovyScriptEngine comment onto its own line before the corresponding cleanup block, and update the applicationContext cleanup around its existing close call to clear the reference in a finally block, matching the nulling behavior already used for scriptFactory and classLoader.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/org/craftercms/engine/service/context/SiteContextFactory.java`:
- Around line 385-391: Update the context cleanup paths in SiteContextFactory so
any exception from storeService.destroyContext(context) is caught and added as a
suppressed exception to the original context-creation exception e, rather than
propagated. Apply this to both the partial-context cleanup inside the destroyEx
catch and the direct cleanup in the else branch, while preserving the existing
cleanup error logging.
---
Nitpick comments:
In `@src/main/java/org/craftercms/engine/service/context/SiteContext.java`:
- Around line 626-633: Move the GroovyScriptEngine comment onto its own line
before the corresponding cleanup block, and update the applicationContext
cleanup around its existing close call to clear the reference in a finally
block, matching the nulling behavior already used for scriptFactory and
classLoader.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e286e1b8-fcf5-460e-8b0a-511c0803e113
📒 Files selected for processing (6)
src/main/java/org/craftercms/engine/scripting/ScriptFactory.javasrc/main/java/org/craftercms/engine/scripting/impl/GroovyScriptFactory.javasrc/main/java/org/craftercms/engine/service/context/SiteContext.javasrc/main/java/org/craftercms/engine/service/context/SiteContextFactory.javasrc/main/java/org/craftercms/engine/service/context/SiteContextManager.javasrc/test/java/org/craftercms/engine/scripting/impl/GroovyScriptFactoryTest.java
|
@coderabbitai is this ready to move forward to a human reviewer? Ignore codacy and travis |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="craftercms/engine"
pr="857"
echo "== Pull request metadata =="
gh pr view "$pr" --repo "$repo" --json number,title,isDraft,mergeable,reviewDecision,statusCheckRollup,reviews \
--jq '{
number,
title,
isDraft,
mergeable,
reviewDecision,
checks: [
.statusCheckRollup[]
| select(
((.name // "") | ascii_downcase | contains("codacy") | not)
and ((.name // "") | ascii_downcase | contains("travis") | not)
)
| {
name,
status: (.status // .state // ""),
conclusion: (.conclusion // ""),
workflow: (.workflowName // "")
}
],
reviews: [
.reviews[]
| {author: .author.login, state, submittedAt}
]
}'
echo
echo "== Unresolved review threads =="
gh api graphql \
-F owner="craftercms" \
-F name="engine" \
-F number="$pr" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
comments(first: 10) {
nodes {
author { login }
body
path
line
originalLine
createdAt
}
}
}
}
}
}
}' \
--jq '
.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false)
| {
path: (.comments.nodes[0].path // ""),
line: (.comments.nodes[0].line // .comments.nodes[0].originalLine // 0),
author: (.comments.nodes[0].author.login // ""),
body: (.comments.nodes[0].body // ""),
createdAt: (.comments.nodes[0].createdAt // "")
}'Length of output: 2563 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="craftercms/engine"
pr="857"
head_sha=$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')
echo "== Head SHA =="
echo "$head_sha"
echo
echo "== Complete status-check rollup =="
gh pr view "$pr" --repo "$repo" --json statusCheckRollup --jq '.statusCheckRollup'
echo
echo "== Check runs =="
gh api "repos/$repo/commits/$head_sha/check-runs?per_page=100" \
--jq '.check_runs[] | {
name,
status,
conclusion,
details_url: .details_url,
app: .app.slug
}'
echo
echo "== Legacy commit statuses =="
gh api "repos/$repo/commits/$head_sha/status" \
--jq '.statuses[] | {
context,
state,
target_url,
creator: .creator.login
}'Length of output: 2531
Before requesting human review, mark it Ready for review since it is currently a draft. GitHub also shows |
2 participants
Groovy class loaders and script engine tweaks to reduce leaks
https://github.com/craftersoftware/craftercms/issues/1266
Summary by CodeRabbit