Repository navigation
fix(core): keep full config on rapid reloads - #1495
Conversation
* fix(core): propagate config reload cancellation
Editing config files in quick succession could leave the plugin
running with a partial config: a superseded reload still published
the sources it had managed to read and notified listeners, so rules
from the remaining sources briefly disappeared.
Root cause: `LayeredConfigReader.reload()` wraps each source in
`runCatching` to isolate failures. A `CancellationException` raised
while waiting on an IntelliJ read action was logged as an ordinary
source failure, so the cancelled reload ran to completion.
`DefaultConfigReader.reload()` then replaced the active delegate and
fired `ConfigReloadListener` without checking whether its coroutine
was still active. The remote config loader can hide the cancellation
entirely: it catches it and returns a fallback.
Solution: rethrow `CancellationException` from the per-source
`onFailure` while keeping warn-and-skip for real source failures,
and call `currentCoroutineContext().ensureActive()` after the new
delegate has loaded but before it becomes active and listeners are
notified. Add tests for both paths plus a guard that ordinary
failures are still isolated. Both guards were reverse-verified:
removing either one makes the new tests fail.
* refactor(psi): extract suspendMergeSort
Move the suspending merge sort out of `DefaultPsiClassHelper.kt` into
a sibling `SuspendMergeSort.kt`, keeping it `internal`. It has no PSI
dependency and already has its own tests. Translate its KDoc and the
comparator test comment to English, matching the rest of the
codebase.
* test(config): move module_path tests to a fixture
The three `${module_path}` cases call `readSync`, which needs an
IntelliJ application. They only passed when another fixture test had
already set one up, so a focused run of `ConfigTextParserTest` failed.
Move them to `ConfigTextParserModulePathTest`, based on the project's
light IDE fixture, so they pass in isolation too.
* docs(psi): translate field ordering comments
Translate the last three Chinese comments added by #1491 at the
`suspendMergeSort` call site. All six Chinese comment lines from that
PR are now English.
Impact: ordinary source-failure isolation is unchanged. Apart from
the two cancellation guards, this branch is test, refactor and
comment only. The changes above address the review on #1491: both
"Before merge" requests and both non-blocking suggestions. Verified
with `./gradlew cleanTest test`: 7669 tests, 0 failures, 0 errors,
10 skipped.
Refs: #1491
|
📦 Plugin has been packaged for this PR. You can download it from the GitHub Actions workflow run by clicking on the "Artifacts" dropdown:
Each artifact is a ready-to-install plugin zip — pick the one matching your IDEA version and install it via Settings → Plugins → ⚙ → Install Plugin from Disk. No extraction needed. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1495 +/- ##
=============================================
+ Coverage 70.114% 70.153% +0.039%
=============================================
Files 525 526 +1
Lines 30499 30502 +3
Branches 7179 7180 +1
=============================================
+ Hits 21384 21398 +14
+ Misses 6350 6345 -5
+ Partials 2765 2759 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Description
Follow-up to the review on #1491 (merged). It implements the two
"Before merge" cancellation guards and both non-blocking suggestions,
and translates the Chinese comments that PR introduced.
A superseded config reload could publish a partial snapshot and notify
listeners with it, briefly dropping entries from sources that had not
been read yet.
LayeredConfigReader.reload()now rethrowsCancellationExceptioninstead of logging it as an ordinary sourcefailure, and
DefaultConfigReader.reload()checkscurrentCoroutineContext().ensureActive()after loading the newdelegate but before making it active. Ordinary source failures are
still isolated and logged.
The remaining changes are review hygiene with no product behaviour
change:
suspendMergeSortmoves to a sibling file, the three${module_path}cases move to an IDE fixture, and the Chinesecomments added by #1491 are now English.
Type of Change
suspendMergeSortmoved to a sibling fileRelated Issues
Fixes # — N/A: no open issue to close; this is a follow-up to a merged PR review.
Refs: #1491 — #1491, review comment 6621840788.
Affected Area
Changes Made
core/config/LayeredConfigReader.kt— rethrowCancellationExceptionfrom the per-sourceonFailure; real source failures still warn and are skipped.core/config/DefaultConfigReader.kt— splitreload()into load,ensureActive(), delegate swap, listener notification.core/config/config/LayeredConfigReaderTest.kt— +2 cases: cancellation propagates and keeps the previous snapshot; ordinary failures remain isolated.core/config/config/DefaultConfigReaderTest.kt— +1 IDE fixture case: a cancelled reload does not notify listeners.core/psi/SuspendMergeSort.kt— new sibling file with theinternalfunction and an English KDoc.core/psi/DefaultPsiClassHelper.kt— function moved out; three Chinese comments at the call site translated.core/psi/SuspendMergeSortTest.kt— Chinese comparator comment translated.core/config/config/parser/ConfigTextParserModulePathTest.kt— new fixture class holding the three${module_path}cases.core/config/config/parser/ConfigTextParserTest.kt— three cases removed, with a pointer to the new class.How I Tested
./gradlew testpasses — run as./gradlew cleanTest test: 7669 tests, 0 failures, 0 errors, 10 skipped, 750 result XMLs. The case count is unchanged from before the migration (3 removed, 3 added)../gradlew runIde) — N/A: no UI entry point changed, and the cancellation paths are covered by unit and IDE-fixture tests. The new fixture class was also run in isolation (3/3 PASSED), as were the two pure JUnit classes (42/42 PASSED), which is the isolation problem the review called out.Reverse verification: both guards were temporarily reverted and the new tests confirmed to fail (including
a cancelled reload must not complete: reloaded), then restored.git diff --checkis clean.Architecture & Threading Checklist
core/configandcore/psipackages;SuspendMergeSort.ktis a sibling of its only caller; no newcore.*→ concrete sibling importsread {}/readSync {}— N/A, no new PSI/VFS reads. The existingreadSyncinConfigTextParser.activeModuleContentRoot()is untouched; only its tests moved to a fixture that provides an Application.write {}/swing {}— N/A, no writes and no UI updateslaunch(Dispatchers.Default)from EDT / startup contexts — none added;ensureActive()only inspects the current coroutine context and starts no workLogging Checklist
LOG.warn; a cancellation is rethrown, not loggedLOG.error,LOG.debug,LOG.trace,println, orprintStackTrace()in production code — none addedrunCatching{}.getOrNull()or emptycatchon meaningful operations —LayeredConfigReaderstill logs the throwable, and cancellation is now rethrown instead of swallowed.CachedResourceResolver.get()has a pre-existingrunCatching{}.getOrNull(); the newensureActive()guards its caller, and changing that resolver is deliberately left out of scope.General Checklist
Screenshots (if applicable)
N/A — no UI change.
Additional Notes