Skip to content

feat(#4850): implement thread-safe caching with ConcurrentCache class#4855

Merged
yegor256 merged 1 commit into
objectionary:masterfrom
volodya-lombrozo:4850-cache-thread-safety
Feb 4, 2026
Merged

feat(#4850): implement thread-safe caching with ConcurrentCache class#4855
yegor256 merged 1 commit into
objectionary:masterfrom
volodya-lombrozo:4850-cache-thread-safety

Conversation

@volodya-lombrozo

@volodya-lombrozo volodya-lombrozo commented Feb 3, 2026

Copy link
Copy Markdown
Member

This PR introduces a ConcurrentCache class to ensure thread-safety for the Cache mechanism, resolving issue #4846.

Fixes #4850

Summary by CodeRabbit

  • Documentation

    • Updated Cache class documentation with thread-safety guidance and best practices.
  • New Features

    • Added thread-safe caching mechanism for concurrent operations.
  • Tests

    • Added test coverage for concurrent caching functionality.

Copilot AI review requested due to automatic review settings February 3, 2026 14:12
@volodya-lombrozo
volodya-lombrozo marked this pull request as draft February 3, 2026 14:12
@coderabbitai

coderabbitai Bot commented Feb 3, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

This PR addresses thread-safety concerns in the Cache class by introducing a new ConcurrentCache wrapper that coordinates access using per-tail locks, enabling concurrent operations across different tails while serializing operations for the same tail. Cache documentation is updated to recommend ConcurrentCache for concurrent scenarios, and a concurrent test validates the locking behavior.

Changes

Cohort / File(s) Summary
Cache Documentation
eo-maven-plugin/src/main/java/org/eolang/maven/Cache.java
Updated Javadoc to document thread-safety limitations and recommend ConcurrentCache for concurrent usage; removed prior thread-safety rationale; updated TODO items for clarity.
ConcurrentCache Implementation
eo-maven-plugin/src/main/java/org/eolang/maven/ConcurrentCache.java
New package-private thread-safe wrapper for Cache using per-tail locking via ConcurrentHashMap. The apply() method acquires a per-tail lock, synchronizes on it, and delegates to the underlying Cache with normalized tail path.
ConcurrentCache Test
eo-maven-plugin/src/test/java/org/eolang/maven/ConcurrentCacheTest.java
New unit test launching 100 concurrent cache applications via Threaded helper, verifying that underlying compilation executes exactly once per tail through counter assertion.

Sequence Diagram

sequenceDiagram
    participant Client
    participant ConcurrentCache
    participant LockMap as Per-Tail Lock Map
    participant Cache
    
    par Concurrent Threads
        Client->>ConcurrentCache: apply(source, target, tail1)
        Client->>ConcurrentCache: apply(source, target, tail1)
        Client->>ConcurrentCache: apply(source, target, tail2)
    end
    
    ConcurrentCache->>LockMap: computeIfAbsent(tail1)
    LockMap-->>ConcurrentCache: lock1
    ConcurrentCache->>ConcurrentCache: synchronized(lock1)
    ConcurrentCache->>Cache: apply(source, target, tail1.normalize())
    Cache-->>ConcurrentCache: done
    
    par Other Thread
        ConcurrentCache->>LockMap: computeIfAbsent(tail2)
        LockMap-->>ConcurrentCache: lock2
        ConcurrentCache->>ConcurrentCache: synchronized(lock2)
        ConcurrentCache->>Cache: apply(source, target, tail2.normalize())
        Cache-->>ConcurrentCache: done
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

Suggested reviewers

  • yegor256

Poem

🐰 A wrapper so wise, with locks per tail,
Threads now dance without fail,
Same tail waits its turn with grace,
Different tails race through space,
Cache is thread-safe at last! 🎉

🚥 Pre-merge checks | ✅ 3 | ❌ 2
❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR implements thread-safe caching via ConcurrentCache class and updates Javadoc to document non-thread-safety of original Cache [#4850]. However, there is no evidence that the puzzle text was removed from the source code as required by the completion criterion. Verify that the puzzle text '4846-67023e31' has been removed from Cache.java line 19-25 to fully satisfy the completion criterion in issue #4850.
Docstring Coverage ⚠️ Warning Docstring coverage is 37.50% 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
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: implementing a thread-safe ConcurrentCache class to address the thread-safety requirement in issue #4850.
Out of Scope Changes check ✅ Passed All changes are directly related to implementing thread-safe caching for issue #4850: ConcurrentCache wrapper, documentation updates, and comprehensive test coverage.

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

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

@github-actions

github-actions Bot commented Feb 3, 2026

Copy link
Copy Markdown
Contributor

🚀 Performance Analysis

All benchmarks are within the acceptable range. No critical degradation detected (threshold is 100%). Please refer to the detailed report for more information.

Click to see the detailed report
Test Base Score PR Score Change % Change Unit Mode
benchmarks.XmirBench.xmirToEO 212.304 203.159 -9.144 -4.31% ms/op Average Time

✅ Performance gain: benchmarks.XmirBench.xmirToEO is faster by 9.144 ms/op (4.31%)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a ConcurrentCache class to provide thread-safe caching functionality, addressing issues #4846 and #4850 where the existing Cache class lacked thread-safety mechanisms. The implementation uses a per-key locking strategy with ConcurrentHashMap to prevent race conditions when multiple threads access the same cache entries concurrently.

Changes:

  • Added ConcurrentCache class that wraps Cache with thread-safe synchronization
  • Added comprehensive test coverage for concurrent cache behavior
  • Added additional tests for Cache to verify behavior with source changes and cache misses
  • Removed puzzle comments from Cache class documentation
  • Updated Cache documentation to reference ConcurrentCache for concurrent scenarios

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.

File Description
eo-maven-plugin/src/main/java/org/eolang/maven/ConcurrentCache.java New thread-safe wrapper class implementing per-key locking for cache operations
eo-maven-plugin/src/main/java/org/eolang/maven/Cache.java Updated documentation to clarify thread-safety and removed resolved puzzle comments
eo-maven-plugin/src/test/java/org/eolang/maven/ConcurrentCacheTest.java New test verifying concurrent cache access compiles source only once
eo-maven-plugin/src/test/java/org/eolang/maven/CacheTest.java Added tests for cache invalidation on source changes, cache miss scenarios, and SHA-256 hash validation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread eo-maven-plugin/src/test/java/org/eolang/maven/CacheTest.java
Comment thread eo-maven-plugin/src/test/java/org/eolang/maven/CacheTest.java
Comment thread eo-maven-plugin/src/test/java/org/eolang/maven/CacheTest.java
Comment thread eo-maven-plugin/src/test/java/org/eolang/maven/CacheTest.java
Comment thread eo-maven-plugin/src/main/java/org/eolang/maven/ConcurrentCache.java Outdated
Comment thread eo-maven-plugin/src/main/java/org/eolang/maven/ConcurrentCache.java
Comment thread eo-maven-plugin/src/main/java/org/eolang/maven/ConcurrentCache.java Outdated
Comment thread eo-maven-plugin/src/main/java/org/eolang/maven/ConcurrentCache.java Outdated
@volodya-lombrozo
volodya-lombrozo force-pushed the 4850-cache-thread-safety branch from 0a5dfc3 to 561e5b9 Compare February 4, 2026 12:53
@volodya-lombrozo
volodya-lombrozo marked this pull request as ready for review February 4, 2026 12:53
@volodya-lombrozo

Copy link
Copy Markdown
Member Author

@yegor256 could you have a look, please?

@yegor256
yegor256 merged commit 534b4f9 into objectionary:master Feb 4, 2026
27 checks passed
@0crat

0crat commented Feb 4, 2026

Copy link
Copy Markdown

@volodya-lombrozo Thanks for the contribution! You've earned +12 points for this: +16 as a basis; +5.8 for hits-of-code (116 × 0.05, capped at 16); -4 for exceeding 100 hits-of-code; -5.8 for excessive review comments. Please, keep them coming. Your running score is +336; don't forget to check your Zerocracy account too.

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.

Cache class lacks thread-safety

4 participants