Repository navigation
ADFA-5422: Load each color scheme once, under a lock - #2129
Daniel-ADFA wants to merge 1 commit into
Conversation
Opening a project restores its editor tabs together, and each editor reads the color scheme on its own IO coroutine. IDEColorSchemeProvider caches the scheme in an unsynchronized lazy getter, so two editors can both call IDEColorScheme.load() on the same instance at once. Both parse into the same MutableIntIntMap and ++colorId counter, which are not thread-safe. parseColorValue() caught any failure from putColor() and returned the raw ARGB color as the color id, so a corrupted write became an out-of-range id that StyleDef.makeStyle() passed to TextStyle.makeStyle(), which throws "color id must be positive and bit count is less than 19" (GlitchTip CODEONTHEGO-K, 42 fatal events, still on C-r-1006). Other interleavings leave the map's probe loop spinning forever. load() is now synchronized and returns once the scheme is loaded, so the second editor waits for the first and reuses its result. Nothing reloads an instance on purpose: load() is only reached through loadColorScheme() while the provider's cached scheme is still null. putColor() failures now propagate to IDEColorSchemeProvider.loadColorScheme(), which logs them and falls back to the default scheme, instead of surfacing later as a crash.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Merge Risk: ⚪ Minimal · up to The change makes color scheme loading safe when several editor tabs load at once, and it routes color registration failures to the existing default-scheme fallback. No concrete merge-blocking risk was found. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Title check The title clearly summarizes the main change: it loads each color scheme once and protects loading with synchronization. Description check The description directly explains the concurrency failure, the code changes, the fallback behavior, and the added tests. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
I’m a rabbit checking colors in the scheme,
One careful load, then no repeats.
Eight little threads can safely arrive,
While parser errors still speak up.
I thump my paws and hop away!
Comment @coderabbitai help to get the list of available commands.
2 participants
ADFA-5422
Fixes the fatal
IllegalArgumentException: color id must be positive and bit count is less than 19fromStyleDef.makeStyle()(GlitchTip CODEONTHEGO-K, 42 events, still on C-r-1006).IDEColorScheme.load()is@Synchronizedand loads once. Opening a project restores its tabs together, and each editor loaded the same scheme instance on its own IO coroutine; the parallel parses corrupted the non-thread-safeMutableIntIntMap.parseColorValue()no longer catches aputColor()failure and returns the raw ARGB color as a color id. That fallback is what turned a corrupted write into this crash; a failure now reachesloadColorScheme(), which logs it and uses the default scheme.IDEColorSchemeLoadTest: concurrent loads of one scheme, and a second load. Without the fix the eight loader threads hang inMutableIntIntMap.findInsertIndexand a second load doubles the color table (35 to 70).Nothing reloads an instance on purpose:
load()is only reached throughIDEColorSchemeProvider.loadColorScheme()while the provider's cached scheme is null, andSchemeParser.parse()builds its own instance.Reproduce
IDEColorSchemeLoadTestwithout the fix.adb shell am set-debug-app -w com.itsaky.androidide, start the app,jdb -attachandstop thread in com.itsaky.androidide.editor.schemes.IDEColorScheme.load$editor_v8Debug. TwoDefaultDispatcherthreads stop inload()at the same time;resumelets them parse in parallel. With the fix the second thread waits on the lock until the first returns.