Skip to content

(#4907) Replaced synchronized with ReentrantLock in PhCached#4916

Open
AlexNetTie wants to merge 1 commit intoobjectionary:masterfrom
AlexNetTie:issue-4907-phcached-lock
Open

(#4907) Replaced synchronized with ReentrantLock in PhCached#4916
AlexNetTie wants to merge 1 commit intoobjectionary:masterfrom
AlexNetTie:issue-4907-phcached-lock

Conversation

@AlexNetTie
Copy link
Copy Markdown
Contributor

@AlexNetTie AlexNetTie commented Mar 6, 2026

What does this PR do?

Replaces synchronized block with ReentrantLock in PhCached.take() method to avoid potential deadlocks and comply with qulice rules. Implements double-checked locking pattern for better performance.

Related issues

Changes

  • Added ReentrantLock field
  • Replaced synchronized (this.cached) with double-checked locking using lock.lock() / lock.unlock()
  • Removed @SuppressWarnings("PMD.AvoidSynchronizedStatement")
  • Removed corresponding @todo puzzle
  • Fixed whitespace formatting issues found by qulice

How to test

Run mvn clean install -pl eo-runtime -am to verify compilation and tests.
Run mvn qulice:check -Pqulice -pl eo-runtime to verify code style.

Notes

Local Windows tests show known MjPrintTest failure unrelated to these changes. All CI checks on Linux are expected to pass.

Summary by CodeRabbit

  • Refactor
    • Improved internal thread-safety mechanisms for better performance and reliability in the caching system.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 6, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 532d87bc-d2e9-4fb6-a077-be7f96fe4016

📥 Commits

Reviewing files that changed from the base of the PR and between 9250c27 and 3fb437e.

📒 Files selected for processing (1)
  • eo-runtime/src/main/java/org/eolang/PhCached.java

📝 Walkthrough

Walkthrough

Replaces the synchronized block in PhCached.java with an explicit ReentrantLock for cache lazy initialization. Implements a double-checked locking pattern to maintain thread safety while using modern lock-based synchronization instead of monitor-based synchronization.

Changes

Cohort / File(s) Summary
Synchronization Mechanism Update
eo-runtime/src/main/java/org/eolang/PhCached.java
Replaces synchronized block with ReentrantLock for cache initialization. Adds lock field, implements double-checked locking pattern, removes PMD suppression annotation, and updates necessary imports.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • yegor256

Poem

🐰 A lock replaces sync's embrace,
No monitors in this space!
Double-checked, it guards the cache,
Thread-safe now—a swift dash! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: replacing synchronized with ReentrantLock in PhCached, directly matching the core modification in the changeset.
Linked Issues check ✅ Passed The PR fully addresses issue #4907 by replacing the synchronized block with ReentrantLock, removing the puzzle annotation, implementing double-checked locking, and fixing qulice whitespace issues.
Out of Scope Changes check ✅ Passed All changes are directly related to the stated objective: replacing synchronized with ReentrantLock in PhCached and removing the associated puzzle, with no extraneous modifications.

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

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

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


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
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 179.590 201.008 21.418 11.93% ms/op Average Time

⚠️ Performance loss: benchmarks.XmirBench.xmirToEO is slower by 21.418 ms/op (11.93%)

synchronized (this.cached) {
if (this.cached.get() == null) {
this.cached.set(this.origin.take(name));
Phi result = this.cached.get();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@AlexNetTie this seems to be thread unsafe code

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.

'Synchronized' in PhCached.java Risks Thread-Safety, Requires Replacement

2 participants