Skip to content

Fix map concurrency bug in UnitsAnnotatedTypeFactory by removing stat…#7620

Open
dimpal14 wants to merge 2 commits intotypetools:masterfrom
dimpal14:fix/units-checker-concurrency
Open

Fix map concurrency bug in UnitsAnnotatedTypeFactory by removing stat…#7620
dimpal14 wants to merge 2 commits intotypetools:masterfrom
dimpal14:fix/units-checker-concurrency

Conversation

@dimpal14
Copy link
Copy Markdown

@dimpal14 dimpal14 commented Apr 5, 2026

Problem :
UnitsAnnotatedTypeFactory used un-synchronized static HashMaps (aliasMap and externalQualsMap). When multiple projects were compiled concurrently in the same JVM (e.g., using Gradle/Maven daemons), this shared global state caused ConcurrentModificationExceptions, compiler freezes, and cross-project memory leaks.

Fixed:
Removed the static modifier from both maps, converting them into instance variables. This correctly restricts the caches to the lifecycle of a single compilation run, ensuring thread-safety and preventing data bleeding between concurrent compilations.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 5, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 470f5772-ac47-4905-8bbe-79668490e7f4

📥 Commits

Reviewing files that changed from the base of the PR and between 753b38f and d583431.

📒 Files selected for processing (1)
  • docs/manual/contributors.tex

📝 Walkthrough

Walkthrough

Changed the scope of externalQualsMap and aliasMap in the Units Checker from class-level static fields to per-instance final fields, so each UnitsAnnotatedTypeFactory maintains its own independent caches with initialization and lifetime tied to the factory instance. Also added a new contributor entry, "Dimpal Singh," to the project documentation.

🚥 Pre-merge checks | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
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.

✏️ 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.

@mernst
Copy link
Copy Markdown
Member

mernst commented Apr 5, 2026

Thanks for this fix.

When multiple projects were compiled concurrently in the same JVM (e.g., using Gradle/Maven daemons),

Can you provide a concrete test case (even if nondeterministic)? If not, is this a problem you have observed, or is it a potential (latent) problem that could occur in the future?

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