Skip to content

Adding code to clear Spring contexts before/after site context init#850

Merged
sumerjabri merged 20 commits into
craftercms:support/4.xfrom
avasquez614:bugfix/8734
Jun 13, 2026
Merged

Adding code to clear Spring contexts before/after site context init#850
sumerjabri merged 20 commits into
craftercms:support/4.xfrom
avasquez614:bugfix/8734

Conversation

@avasquez614

Copy link
Copy Markdown
Member

Alfonso Vasquez added 20 commits March 25, 2025 19:00
This reverts commit f1156d0.
@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0a33f7c6-fa93-4a2f-a17f-be7bddce19fb

📥 Commits

Reviewing files that changed from the base of the PR and between ba965dd and e12a5ce.

📒 Files selected for processing (1)
  • src/main/java/org/craftercms/engine/service/context/SiteContext.java

Summary by CodeRabbit

  • Bug Fixes
    • Improved thread context handling in maintenance task execution to prevent context isolation issues between concurrent operations.

Walkthrough

SiteContext now configures its maintenance task executor using a custom ThreadPoolExecutor that clears Spring request and locale thread contexts before and after each maintenance task execution, replacing the previous plain Executors.newSingleThreadExecutor(). Error logging is centralized in the afterExecute hook.

Changes

Spring Thread Context Cleanup for Maintenance Executor

Layer / File(s) Summary
Maintenance executor with Spring thread context cleanup
src/main/java/org/craftercms/engine/service/context/SiteContext.java
Imports for LocaleContextHolder and RequestContextHolder are added; SiteContext constructor delegates executor creation to a new createMaintenanceTaskExecutor() factory method that configures a single-thread executor with beforeExecute/afterExecute hooks to clear Spring thread-bound contexts before and after task execution; centralized error logging in afterExecute when tasks fail.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Suggested reviewers

  • sumerjabri
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description only contains a GitHub issue link without explaining what the PR does, why it's needed, or how it solves the problem. Expand the description to explain the issue being fixed, the solution implemented, and any relevant context or testing information.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding code to clear Spring contexts before/after site context initialization, which aligns with the file summary showing ThreadPoolExecutor clearing request and locale contexts.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@avasquez614 avasquez614 marked this pull request as ready for review June 12, 2026 18:34
@avasquez614 avasquez614 requested a review from sumerjabri as a code owner June 12, 2026 18:34
@sumerjabri sumerjabri requested a review from jmendeza June 12, 2026 19:40
@sumerjabri sumerjabri merged commit c2a9976 into craftercms:support/4.x Jun 13, 2026
2 of 4 checks passed
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