fix(#4904): synchronized replaced with lock in Files#4941
fix(#4904): synchronized replaced with lock in Files#4941eshabakhov wants to merge 5 commits intoobjectionary:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaced synchronized access to the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ 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 |
eshabakhov
changed the title
🚀 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
✅ Performance gain: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/main/java/EOorg/EOeolang/EOfs/Files.java`:
- Line 21: The Files class declares a field as "Lock lock" but only imports
ReentrantLock, causing a compilation error; fix by either adding the missing
import "java.util.concurrent.locks.Lock" or change the field declaration to use
"ReentrantLock lock" so the type matches the existing import; update the
declaration in the Files class where the field "lock" is defined to use the
chosen type consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e1d29e6-89d8-4e11-8822-d014c7922f9f
📒 Files selected for processing (1)
eo-runtime/src/main/java/EOorg/EOeolang/EOfs/Files.java
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/main/java/EOorg/EOeolang/EOfs/Files.java`:
- Around line 112-115: In the Files.write code path where it checks
this.streams.containsKey(name) and throws new ExFailure(..., name), update the
error message string to say "File output stream with name %s is absent, can't
write" (replace "can't read" with "can't write") so the ExFailure thrown by the
write method accurately reflects the operation and uses the same ExFailure
constructor call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6b652c8-6150-41e5-a0b1-7ead309a6e14
📒 Files selected for processing (1)
eo-runtime/src/main/java/EOorg/EOeolang/EOfs/Files.java
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
eo-runtime/src/main/java/EOorg/EOeolang/EOfs/Files.java (2)
29-35: Remove resolved TODO and obsolete suppression.
@todo#4884`` is already implemented, and@SuppressWarnings("PMD.AvoidSynchronizedStatement")is now stale. Keeping both is misleading and weakens future static checks.🧹 Suggested cleanup
- * `@todo` `#4884`:30min Use ReentrantLock instead of 'synchronized' in Files. - * We should use ReentrantLock instead of 'synchronized' to avoid potential - * deadlocks when multiple AtOnce attributes are used together. - * Moreover, 'synchronized' keyword is forbidden by qulice. */ -@SuppressWarnings("PMD.AvoidSynchronizedStatement") final class Files {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eo-runtime/src/main/java/EOorg/EOeolang/EOfs/Files.java` around lines 29 - 35, Remove the now-resolved `@todo` and the obsolete suppression: delete the comment block containing "@todo `#4884`:30min Use ReentrantLock instead of 'synchronized' in Files." and remove the `@SuppressWarnings`("PMD.AvoidSynchronizedStatement") annotation from the Files class declaration so the code no longer contains stale TODOs or misleading suppression; ensure the class declaration reads simply "final class Files {" (leave other code unchanged).
86-148: Add concurrent regression tests for the new lock-based behavior.This PR changes synchronization semantics, but
eo-runtime/src/test/java/EOorg/EOeolang/EOfs/FilesTest.javacurrently validates only single-threaded flows. Please add tests for concurrentread/write/closeinterleavings to protect this change from regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eo-runtime/src/main/java/EOorg/EOeolang/EOfs/Files.java` around lines 86 - 148, The Files class introduced lock-based synchronization for stream operations (methods read, write, close) but lacks concurrent tests; add multithreaded regression tests in EOorg/EOeolang/EOfs/FilesTest to exercise interleavings of Files.read, Files.write, and Files.close (and opening if needed) under concurrent load, asserting correctness and absence of races/IOExceptions where appropriate; use multiple threads (Executors/Futures), coordinate with latches/barriers to force interleavings, perform repeated iterations to catch flakiness, and include tests that ensure closed streams cause expected ExFailure while concurrent reads/writes produce consistent data to prevent regressions.
🤖 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/EOorg/EOeolang/EOfs/Files.java`:
- Around line 55-58: open() currently mutates this.streams without acquiring
this.lock while read(), write(), and close() do acquire the lock, causing a race
that can create duplicate streams; modify open() to follow the same lock pattern
as read()/write()/close(): acquire this.lock before checking/using this.streams,
verify the key is still absent (or present as appropriate) before creating and
putting new InputStream/OutputStream entries via putIfAbsent, and ensure the
lock is released in a finally block so only one thread can create streams for a
given key and avoid leaking resources.
---
Nitpick comments:
In `@eo-runtime/src/main/java/EOorg/EOeolang/EOfs/Files.java`:
- Around line 29-35: Remove the now-resolved `@todo` and the obsolete suppression:
delete the comment block containing "@todo `#4884`:30min Use ReentrantLock instead
of 'synchronized' in Files." and remove the
`@SuppressWarnings`("PMD.AvoidSynchronizedStatement") annotation from the Files
class declaration so the code no longer contains stale TODOs or misleading
suppression; ensure the class declaration reads simply "final class Files {"
(leave other code unchanged).
- Around line 86-148: The Files class introduced lock-based synchronization for
stream operations (methods read, write, close) but lacks concurrent tests; add
multithreaded regression tests in EOorg/EOeolang/EOfs/FilesTest to exercise
interleavings of Files.read, Files.write, and Files.close (and opening if
needed) under concurrent load, asserting correctness and absence of
races/IOExceptions where appropriate; use multiple threads (Executors/Futures),
coordinate with latches/barriers to force interleavings, perform repeated
iterations to catch flakiness, and include tests that ensure closed streams
cause expected ExFailure while concurrent reads/writes produce consistent data
to prevent regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 051621bb-ccf6-4051-a9cd-49bf803a53fe
📒 Files selected for processing (1)
eo-runtime/src/main/java/EOorg/EOeolang/EOfs/Files.java
| private Files() { | ||
| this.streams = new ConcurrentHashMap<>(0); | ||
| this.lock = new ReentrantLock(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify lock usage consistency across Files methods (read-only).
fd 'Files.java$' -t f | while read -r file; do
echo "=== $file ==="
rg -n -C2 'void open\(|void read\(|void write\(|void close\(|this\.lock\.lock\(' "$file"
doneRepository: objectionary/eo
Length of output: 1124
Guard open() with the same lock to keep synchronization consistent.
read(), write(), and close() now acquire this.lock, but open() mutates this.streams without it. This creates a race condition: multiple threads can simultaneously check putIfAbsent(), both open the file streams, and leak resources. Wrap open() in the same lock pattern and verify the key exists before creating streams.
🔧 Suggested fix
void open(final String name) throws IOException {
- final Path path = Paths.get(name);
- this.streams.putIfAbsent(
- name,
- new Object[]{
- java.nio.file.Files.newInputStream(path),
- java.nio.file.Files.newOutputStream(path, StandardOpenOption.APPEND),
- }
- );
+ this.lock.lock();
+ try {
+ if (this.streams.containsKey(name)) {
+ return;
+ }
+ final Path path = Paths.get(name);
+ this.streams.put(
+ name,
+ new Object[]{
+ java.nio.file.Files.newInputStream(path),
+ java.nio.file.Files.newOutputStream(path, StandardOpenOption.APPEND),
+ }
+ );
+ } 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/EOorg/EOeolang/EOfs/Files.java` around lines 55 -
58, open() currently mutates this.streams without acquiring this.lock while
read(), write(), and close() do acquire the lock, causing a race that can create
duplicate streams; modify open() to follow the same lock pattern as
read()/write()/close(): acquire this.lock before checking/using this.streams,
verify the key is still absent (or present as appropriate) before creating and
putting new InputStream/OutputStream entries via putIfAbsent, and ensure the
lock is released in a finally block so only one thread can create streams for a
given key and avoid leaking resources.
1 participant
Summary
This PR solves #4904
Changes
synchronizedstatements replaced withlockin Files.javaTest
Run the following commands to verify:
Summary by CodeRabbit