#4673 Make AtWithRho.get() thread-safe#5023
Open
idrisoffrinat-cpu wants to merge 5 commits intoobjectionary:masterfrom
Open
#4673 Make AtWithRho.get() thread-safe#5023idrisoffrinat-cpu wants to merge 5 commits intoobjectionary:masterfrom
idrisoffrinat-cpu wants to merge 5 commits intoobjectionary:masterfrom
Conversation
Cache the first-fixup result in an AtomicReference guarded by a ReentrantLock (double-checked locking). Concurrent callers now receive the exact same Phi instance instead of each creating its own copy and setting RHO on it. put() invalidates the cache so subsequent get() reflects the new underlying value. Add a concurrent test that spawns 16 threads hitting get() simultaneously and asserts a single distinct instance is returned. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds a thread-safe, memoized initialization to AtWithRho: get() now caches a single Phi using an AtomicReference and ReentrantLock (double-checked locking) and ensures Phi.RHO is attached; put() invalidates the cache. A concurrent test verifies callers receive the same Phi instance. Changes
Sequence Diagram(s)sequenceDiagram
participant Callers as Caller Threads
participant At as AtWithRho
participant Orig as original Attr
participant Phi as Phi instance
Callers->>At: get()
At->>At: cached.get() (check)
alt cached present
At-->>Callers: return cached
else cached absent
At->>At: lock()
At->>At: cached.get() (double-check)
alt still absent
At->>Orig: original.get()
Orig-->>At: rawPhi
At->>At: ensure RHO attached (copy if needed)
At->>At: cached.set(phiWithRho)
end
At->>At: unlock()
At-->>Callers: return cached
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
🚀 Performance AnalysisAll 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@eo-runtime/src/main/java/org/eolang/AtWithRho.java`:
- Around line 27-30: The class AtWithRho currently stores every result into the
AtomicReference cached inside get(), turning pass-through origins into
unconditional caches; change get() so it only writes cached when you create the
"fixed-up" copy for the missing-RHO case (the branch that constructs a new Phi
with put(RHO, ...) or similar). Concretely: in AtWithRho.get() check if RHO is
present on origin first and return that value without writing cached; only when
you detect RHO is missing and you build the fixed-up Phi do you set
cached.set(fixedUpPhi) and return it. Keep the cached field and its usage
restricted to the fixed-up creation path so origins that produce varying values
remain pass-through.
- Around line 80-82: put() clears cached without holding the same lock used by
get(), allowing a race where get() recomputes and rewrites a stale value; fix by
wrapping the invalidation and delegation inside the same synchronization used by
get() (i.e., use the same lock object/monitor that guards cached initialization
in get()) — for example, place cached.set(null) and original.put(phi) inside a
synchronized(...) block using the lock object referenced in get() so put() is
serialized with get()'s initialization.
In `@eo-runtime/src/test/java/org/eolang/AtWithRhoTest.java`:
- Around line 7-12: The test currently submits tasks with ExecutorService.submit
but never inspects the returned Futures and uses a Set (results) which relies on
equals/hashCode; change the test in AtWithRhoTest to collect the Future<?>
objects returned by submit, after awaiting the CountDownLatch iterate over each
Future and call get (with a timeout) so any worker exceptions are thrown and
fail the test, and replace the Set-based uniqueness/assertion with explicit
identity checks (use assertSame or Hamcrest sameInstance) against the expected
object instances for every result entry instead of relying on Set membership;
refer to the variables/methods ExecutorService.submit, Future.get,
CountDownLatch.await, and the results collection to locate where to change code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f69aba0-5079-4423-8e13-92cb2151235c
📒 Files selected for processing (2)
eo-runtime/src/main/java/org/eolang/AtWithRho.javaeo-runtime/src/test/java/org/eolang/AtWithRhoTest.java
Comment on lines
80
to
82
| public void put(final Phi phi) { | ||
| this.cached.set(null); | ||
| this.original.put(phi); |
Contributor
There was a problem hiding this comment.
Synchronize cache invalidation with initialization.
put() clears cached without the get() lock. A racing get() can compute an old value, put() can clear/update, then get() can write the stale value back into cached. Use the same lock around invalidation and delegation.
🔒 Proposed fix: serialize `put()` with `get()` initialization
`@Override`
public void put(final Phi phi) {
- this.cached.set(null);
- this.original.put(phi);
+ this.lock.lock();
+ try {
+ this.cached.set(null);
+ this.original.put(phi);
+ } finally {
+ this.lock.unlock();
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@eo-runtime/src/main/java/org/eolang/AtWithRho.java` around lines 80 - 82,
put() clears cached without holding the same lock used by get(), allowing a race
where get() recomputes and rewrites a stale value; fix by wrapping the
invalidation and delegation inside the same synchronization used by get() (i.e.,
use the same lock object/monitor that guards cached initialization in get()) —
for example, place cached.set(null) and original.put(phi) inside a
synchronized(...) block using the lock object referenced in get() so put() is
serialized with get()'s initialization.
Comment on lines
+7
to
+12
| import java.util.Set; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
| import java.util.concurrent.CountDownLatch; | ||
| import java.util.concurrent.ExecutorService; | ||
| import java.util.concurrent.Executors; | ||
| import java.util.concurrent.TimeUnit; |
Contributor
There was a problem hiding this comment.
Surface worker failures and assert identity directly.
submit() captures task exceptions in Futures, but this test never reads them. If only one worker succeeds and the rest fail, results can still have size 1. Also, Set uniqueness uses equals/hashCode, not strict instance identity.
🧪 Proposed fix: collect futures and assert `sameInstance` for every result
+import java.util.ArrayList;
+import java.util.List;
-import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
@@
`@Test`
- void returnsSameInstanceToConcurrentCallers() throws InterruptedException {
+ void returnsSameInstanceToConcurrentCallers() throws Exception {
final int threads = 16;
final Attr attr = new AtWithRho(
new AtComposite(new PhDefault(), phi -> phi),
new PhDefault()
);
- final Set<Phi> results = ConcurrentHashMap.newKeySet();
final CountDownLatch start = new CountDownLatch(1);
- final CountDownLatch done = new CountDownLatch(threads);
final ExecutorService pool = Executors.newFixedThreadPool(threads);
+ final List<Future<Phi>> futures = new ArrayList<>(threads);
try {
for (int idx = 0; idx < threads; ++idx) {
- pool.submit(() -> {
- try {
- start.await();
- results.add(attr.get());
- } catch (final InterruptedException ex) {
- Thread.currentThread().interrupt();
- } finally {
- done.countDown();
- }
+ futures.add(pool.submit(() -> {
+ start.await();
+ return attr.get();
});
}
start.countDown();
+ final Phi first = futures.get(0).get(10L, TimeUnit.SECONDS);
+ for (final Future<Phi> future : futures) {
+ MatcherAssert.assertThat(
+ "AtWithRho.get() must return the same instance for all concurrent callers",
+ future.get(10L, TimeUnit.SECONDS),
+ Matchers.sameInstance(first)
+ );
+ }
- MatcherAssert.assertThat(
- "all threads must finish",
- done.await(10L, TimeUnit.SECONDS),
- Matchers.is(true)
- );
} finally {
pool.shutdownNow();
}
- MatcherAssert.assertThat(
- "AtWithRho.get() must return the same instance for all concurrent callers",
- results,
- Matchers.hasSize(1)
- );
}Also applies to: 107-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@eo-runtime/src/test/java/org/eolang/AtWithRhoTest.java` around lines 7 - 12,
The test currently submits tasks with ExecutorService.submit but never inspects
the returned Futures and uses a Set (results) which relies on equals/hashCode;
change the test in AtWithRhoTest to collect the Future<?> objects returned by
submit, after awaiting the CountDownLatch iterate over each Future and call get
(with a timeout) so any worker exceptions are thrown and fail the test, and
replace the Set-based uniqueness/assertion with explicit identity checks (use
assertSame or Hamcrest sameInstance) against the expected object instances for
every result entry instead of relying on Set membership; refer to the
variables/methods ExecutorService.submit, Future.get, CountDownLatch.await, and
the results collection to locate where to change code.
Split AtWithRho.get() into helper methods to flatten nested ifs, and restructure the concurrent test to use try-with-resources on ExecutorService with a single assertion via a helper collector. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Author
|
Thanks @coderabbitai for flagging the issue-link mismatch. To clarify:
The Also just pushed a follow-up commit fixing the 8 qulice violations that broke the previous build (nested-if depth in |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 673 🏁 Script executed: Length of output: 2129 🏁 Script executed: Length of output: 12516
After checking issue Recommended steps:
✏️ Learnings added
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
eo-runtime/src/test/java/org/eolang/AtWithRhoTest.java (1)
7-11:⚠️ Potential issue | 🟠 MajorRead worker
Futures and assert identity directly.Line 135 discards
submit()results, so worker failures are hidden. Line 117 also verifies set uniqueness, not strict instance identity. CollectFuture<Phi>values and assertsameInstancefor every result.🧪 Proposed fix: collect futures and compare returned instances
-import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; +import java.util.ArrayList; +import java.util.List; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; @@ `@Test` - void returnsSameInstanceToConcurrentCallers() throws InterruptedException { + void returnsSameInstanceToConcurrentCallers() throws Exception { + final List<Phi> results = AtWithRhoTest.collectConcurrentGet( + new AtWithRho( + new AtComposite(new PhDefault(), phi -> phi), + new PhDefault() + ), + 16 + ); + final Phi first = results.get(0); MatcherAssert.assertThat( "AtWithRho.get() must return the same instance for all concurrent callers", - AtWithRhoTest.collectConcurrentGet( - new AtWithRho( - new AtComposite(new PhDefault(), phi -> phi), - new PhDefault() - ), - 16 - ), - Matchers.hasSize(1) + results, + Matchers.everyItem(Matchers.sameInstance(first)) ); } @@ - * `@return` Distinct {`@link` Phi} instances returned across threads - * `@throws` InterruptedException If interrupted while waiting + * `@return` {`@link` Phi} instances returned across threads + * `@throws` Exception If a worker fails or times out */ - private static Set<Phi> collectConcurrentGet(final Attr attr, final int threads) - throws InterruptedException { - final Set<Phi> results = ConcurrentHashMap.newKeySet(); + private static List<Phi> collectConcurrentGet(final Attr attr, final int threads) + throws Exception { final CountDownLatch start = new CountDownLatch(1); + final List<Future<Phi>> futures = new ArrayList<>(threads); try (ExecutorService pool = Executors.newFixedThreadPool(threads)) { for (int idx = 0; idx < threads; ++idx) { - pool.submit( + futures.add( + pool.submit( () -> { - try { - start.await(); - results.add(attr.get()); - } catch (final InterruptedException ex) { - Thread.currentThread().interrupt(); - } + start.await(); + return attr.get(); } + ) ); } start.countDown(); + final List<Phi> results = new ArrayList<>(threads); + for (final Future<Phi> future : futures) { + results.add(future.get(10L, TimeUnit.SECONDS)); + } + return results; } - return results; }Also applies to: 106-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eo-runtime/src/test/java/org/eolang/AtWithRhoTest.java` around lines 7 - 11, The test currently discards ExecutorService.submit() results and only checks collection uniqueness; change it to collect Future<Phi> from each submit (type Future<Phi>), wait for all futures via future.get(), and assert instance identity (use assertSame or ==) for each returned Phi rather than Set uniqueness; update the logic around the worker submission loop in AtWithRhoTest (the test method that uses ExecutorService, ConcurrentHashMap/Set and CountDownLatch) to store submissions in a List<Future<Phi>> and then iterate that list to compare returned instances directly to ensure strict same-instance semantics.eo-runtime/src/main/java/org/eolang/AtWithRho.java (2)
27-30:⚠️ Potential issue | 🟠 MajorCache only the fixed-up copy, not already-bound RHO values.
Line 82 caches the result even when
withRho()returns the original object from Lines 96-97. That makesAtWithRhocache pass-through attributes that may legitimately return differentPhiinstances across calls.🐛 Proposed fix: restrict caching to the missing-RHO branch
/** - * Cached result of {`@link` `#get`()} to guarantee a consistent view across threads. + * Cached fixed-up result when the original object has no {`@link` Phi#RHO}. */ private final AtomicReference<Phi> cached; @@ /** * Initialize the cached value under the lock, using double-checked locking. - * `@return` Cached {`@link` Phi} + * `@return` {`@link` Phi} with {`@link` Phi#RHO} attached */ private Phi initialize() { this.lock.lock(); try { Phi ret = this.cached.get(); if (ret == null) { - ret = this.withRho(this.original.get()); - this.cached.set(ret); + final Phi origin = this.original.get(); + if (origin.hasRho()) { + ret = origin; + } else { + ret = this.withRho(origin); + this.cached.set(ret); + } } return ret; } finally { this.lock.unlock(); } @@ /** - * Return the given object with {`@link` Phi#RHO} attached, copying it if needed. + * Return a copy of the given object with {`@link` Phi#RHO} attached. * `@param` phi Original object * `@return` Object with {`@code` \rho} set */ private Phi withRho(final Phi phi) { - if (phi.hasRho()) { - return phi; - } final Phi copy = phi.copy(); copy.put(Phi.RHO, this.rho); return copy; }Also applies to: 76-82, 95-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eo-runtime/src/main/java/org/eolang/AtWithRho.java` around lines 27 - 30, AtWithRho is currently caching every result of get(), including pass-through values returned unchanged by withRho(), which prevents legitimate different Phi instances from being returned later; modify get() so it retrieves the original Phi (call it orig), then computes fixed = withRho(orig) and only stores fixed into the AtomicReference cached when fixed != orig (i.e., when withRho actually produced a fixed-up copy); if fixed == orig return it directly without updating cached, otherwise cache fixed (use the existing cached.compareAndSet/null set logic) and return fixed. Ensure you reference and update the existing cached field, the get() method, and the withRho() call sites accordingly.
67-69:⚠️ Potential issue | 🔴 CriticalSerialize
put()with cache initialization.
put()can clear the cache while a racingget()is computing an old fixed-up value, after whichget()may write the stale value back. Use the same lock for invalidation and delegation.🔒 Proposed fix: guard cache invalidation with the initialization lock
`@Override` public void put(final Phi phi) { - this.cached.set(null); - this.original.put(phi); + this.lock.lock(); + try { + this.cached.set(null); + this.original.put(phi); + } finally { + this.lock.unlock(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eo-runtime/src/main/java/org/eolang/AtWithRho.java` around lines 67 - 69, AtWithRho.put currently clears cached (this.cached.set(null)) then calls original.put(phi), which can race with get()’s initialization; guard both the cache invalidation and the delegation to original.put under the same initialization lock used by get() so the invalidation and potential recomputation cannot interleave. Concretely, acquire the same initialization lock used in get() (the class's initialization/initializationLock or equivalent), perform this.cached.set(null) and original.put(phi) while holding that lock, then release it; this ensures put(), get(), cached, and original.put are serialized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@eo-runtime/src/main/java/org/eolang/AtWithRho.java`:
- Around line 27-30: AtWithRho is currently caching every result of get(),
including pass-through values returned unchanged by withRho(), which prevents
legitimate different Phi instances from being returned later; modify get() so it
retrieves the original Phi (call it orig), then computes fixed = withRho(orig)
and only stores fixed into the AtomicReference cached when fixed != orig (i.e.,
when withRho actually produced a fixed-up copy); if fixed == orig return it
directly without updating cached, otherwise cache fixed (use the existing
cached.compareAndSet/null set logic) and return fixed. Ensure you reference and
update the existing cached field, the get() method, and the withRho() call sites
accordingly.
- Around line 67-69: AtWithRho.put currently clears cached
(this.cached.set(null)) then calls original.put(phi), which can race with
get()’s initialization; guard both the cache invalidation and the delegation to
original.put under the same initialization lock used by get() so the
invalidation and potential recomputation cannot interleave. Concretely, acquire
the same initialization lock used in get() (the class's
initialization/initializationLock or equivalent), perform this.cached.set(null)
and original.put(phi) while holding that lock, then release it; this ensures
put(), get(), cached, and original.put are serialized.
In `@eo-runtime/src/test/java/org/eolang/AtWithRhoTest.java`:
- Around line 7-11: The test currently discards ExecutorService.submit() results
and only checks collection uniqueness; change it to collect Future<Phi> from
each submit (type Future<Phi>), wait for all futures via future.get(), and
assert instance identity (use assertSame or ==) for each returned Phi rather
than Set uniqueness; update the logic around the worker submission loop in
AtWithRhoTest (the test method that uses ExecutorService, ConcurrentHashMap/Set
and CountDownLatch) to store submissions in a List<Future<Phi>> and then iterate
that list to compare returned instances directly to ensure strict same-instance
semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5dec2874-4ce8-4e35-a3ca-2a32a470e5c6
📒 Files selected for processing (2)
eo-runtime/src/main/java/org/eolang/AtWithRho.javaeo-runtime/src/test/java/org/eolang/AtWithRhoTest.java
…rules Rewrite AtWithRho.get() and withRho() to use a single return, and return the distinct-count (int) from the test helper instead of the sink Set to avoid qulice's UnnecessaryLocalRule on the collection. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@eo-runtime/src/test/java/org/eolang/AtWithRhoTest.java`:
- Around line 129-130: The method signature for distinctConcurrentGets declares
a throws InterruptedException even though nothing inside (including the worker
lambda's caught exceptions, start.countDown(), and ExecutorService.close())
throws it; remove the "throws InterruptedException" from the
distinctConcurrentGets method declaration (the method named
distinctConcurrentGets) and update any callers if they relied on that checked
exception (they should not need changes since no InterruptedException is
propagated).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4ad8512-a222-4aa5-a671-0b800d1445bd
📒 Files selected for processing (2)
eo-runtime/src/main/java/org/eolang/AtWithRho.javaeo-runtime/src/test/java/org/eolang/AtWithRhoTest.java
✅ Files skipped from review due to trivial changes (1)
- eo-runtime/src/main/java/org/eolang/AtWithRho.java
Comment on lines
+129
to
+130
| private static int distinctConcurrentGets(final Attr attr, final int threads) | ||
| throws InterruptedException { |
Contributor
There was a problem hiding this comment.
Remove the unused throws InterruptedException.
Nothing in the method body throws InterruptedException: start.await() lives inside the worker lambda (and is caught there), start.countDown() never throws, and ExecutorService.close() does not declare it. SonarCloud flags this as a dead declaration.
🔧 Proposed fix
- private static int distinctConcurrentGets(final Attr attr, final int threads)
- throws InterruptedException {
+ private static int distinctConcurrentGets(final Attr attr, final int threads) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private static int distinctConcurrentGets(final Attr attr, final int threads) | |
| throws InterruptedException { | |
| private static int distinctConcurrentGets(final Attr attr, final int threads) { |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 130-130: Remove the declaration of thrown exception 'java.lang.InterruptedException', as it cannot be thrown from method's body.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@eo-runtime/src/test/java/org/eolang/AtWithRhoTest.java` around lines 129 -
130, The method signature for distinctConcurrentGets declares a throws
InterruptedException even though nothing inside (including the worker lambda's
caught exceptions, start.countDown(), and ExecutorService.close()) throws it;
remove the "throws InterruptedException" from the distinctConcurrentGets method
declaration (the method named distinctConcurrentGets) and update any callers if
they relied on that checked exception (they should not need changes since no
InterruptedException is propagated).
… a parameter Qulice's UnnecessaryLocalRule applies only to local variables, not to method parameters. Split the concurrent helper into a factory and a worker that accepts the sink collection as an argument, keeping the test equivalent while satisfying the lint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
eo-runtime/src/test/java/org/eolang/AtWithRhoTest.java (2)
129-130:⚠️ Potential issue | 🟡 MinorDrop the unused
throws InterruptedExceptionon both helpers.Neither method body throws
InterruptedException:start.await()is inside the worker lambda (and handled there),start.countDown()does not throw, andExecutorService.close()does not declare it. SonarCloud still flags line 143; the same is true for line 130.🔧 Proposed fix
- private static int distinctConcurrentGets(final Attr attr, final int threads) - throws InterruptedException { + private static int distinctConcurrentGets(final Attr attr, final int threads) { return AtWithRhoTest.distinctGets(attr, threads, ConcurrentHashMap.newKeySet()); } @@ - private static int distinctGets(final Attr attr, final int threads, final Set<Phi> sink) - throws InterruptedException { + private static int distinctGets(final Attr attr, final int threads, final Set<Phi> sink) {And correspondingly drop
throws InterruptedExceptionfromreturnsSameInstanceToConcurrentCallersat line 107.Also applies to: 142-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eo-runtime/src/test/java/org/eolang/AtWithRhoTest.java` around lines 129 - 130, Remove the unnecessary checked exception declaration from the two helper methods by deleting "throws InterruptedException" from the method signatures of distinctConcurrentGets(Attr, int) and returnsSameInstanceToConcurrentCallers(...) (the helpers flagged by SonarCloud); neither method body throws InterruptedException (start.await() is handled inside the worker lambda and start.countDown()/ExecutorService.close() do not declare it), so update the method signatures to no longer declare the checked exception.
142-161:⚠️ Potential issue | 🟠 MajorWorker failures are swallowed and
Setusesequals/hashCode, not identity.Two concerns in
distinctGetsthat can let the test pass incorrectly:
pool.submit(Runnable)returnsFutures that are never inspected. Ifattr.get()throws aRuntimeExceptionon most workers and succeeds on only one,sink.size()is still1and the test passes — masking a regression. Collect theFutures and callget()on each so exceptions propagate.- Uniqueness via
ConcurrentHashMap.newKeySet()relies onPhi.equals/hashCode. IfPhi(or a subclass) ever overridesequalsto compare by logical content, two distinct instances could collapse to one set element and the assertion would falsely pass. The intent here is instance identity — useMatchers.sameInstanceagainst a reference value captured from the firstFuture.get().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eo-runtime/src/test/java/org/eolang/AtWithRhoTest.java` around lines 142 - 161, The test swallows worker exceptions and relies on logical equality instead of identity; in distinctGets change the worker submission to collect the Futures returned by pool.submit(...) and after start.countDown() iterate each Future calling get() so any exceptions propagate; gather each Future.get() result (Phi) into an identity-based set (e.g., Collections.newSetFromMap(new IdentityHashMap<>())) instead of the current sink that relies on equals/hashCode, and return that identity-set's size; update references to Attr.get(), pool, start, and sink in the distinctGets method accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@eo-runtime/src/test/java/org/eolang/AtWithRhoTest.java`:
- Around line 129-130: Remove the unnecessary checked exception declaration from
the two helper methods by deleting "throws InterruptedException" from the method
signatures of distinctConcurrentGets(Attr, int) and
returnsSameInstanceToConcurrentCallers(...) (the helpers flagged by SonarCloud);
neither method body throws InterruptedException (start.await() is handled inside
the worker lambda and start.countDown()/ExecutorService.close() do not declare
it), so update the method signatures to no longer declare the checked exception.
- Around line 142-161: The test swallows worker exceptions and relies on logical
equality instead of identity; in distinctGets change the worker submission to
collect the Futures returned by pool.submit(...) and after start.countDown()
iterate each Future calling get() so any exceptions propagate; gather each
Future.get() result (Phi) into an identity-based set (e.g.,
Collections.newSetFromMap(new IdentityHashMap<>())) instead of the current sink
that relies on equals/hashCode, and return that identity-set's size; update
references to Attr.get(), pool, start, and sink in the distinctGets method
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e86b886-e0eb-426e-a4f8-6794d1cb945e
📒 Files selected for processing (1)
eo-runtime/src/test/java/org/eolang/AtWithRhoTest.java
…rho-thread-safe # Conflicts: # eo-runtime/src/main/java/org/eolang/AtWithRho.java
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the race condition in
AtWithRho.get()described in #4673. Previously, when the underlying object lacked RHO, concurrent callers would each:!ret.hasRho()checkret.copy()…so each thread received a different
Phiinstance, violating the expectation of a consistent view.Fix
AtomicReference<Phi>guarded by aReentrantLock(double-checked locking — same pattern used inPhCached/PhOnce)put()invalidates the cache (cached.set(null)) so a subsequentget()reflects the new underlying valueThis attribute is NOT thread safe!note from the class javadocTest plan
returnsSameInstanceToConcurrentCallers— spawns 16 threads hittingget()simultaneously from a shared start-gate and asserts exactly one distinct instance is returnedAtWithRhoTestcases still passmvn test -pl eo-runtimepasses with no regressionsCloses #4673
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Tests