bug(#4891): synchronized block replacement with ReentrantLock in StrictXmir#4895
Conversation
📝 WalkthroughWalkthroughThe StrictXmir class synchronization mechanism is refactored to use a static reentrant lock instead of per-call synchronization blocks. The tmp parameter is removed from fetch, copied, and downloaded methods, simplifying their signatures. Related imports and exception handling are updated accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
Tip Issue Planner is now in beta. Read the docs and try it out! 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. Comment |
🚀 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}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
eo-parser/src/main/java/org/eolang/parser/StrictXmir.java (2)
80-85:⚠️ Potential issue | 🟡 MinorStale Javadoc on second constructor
The comment "Synchronization by XML is necessary…" no longer describes the actual mechanism. Per-instance thread safety is now handled by
Synced<>(Sticky<>(...)), and cross-instance file-write synchronization is via the staticLOCK— not by locking on theXMLobject.📝 Proposed fix
- /** - * Ctor. - * Synchronization by XML is necessary in case we're trying to validate the same - * {`@link` XML} in multiple threads. In such case the path to XSD scheme inside XML should - * be updated only once. - * `@param` before The XML source - * `@param` tmp The directory with cached XSD files - */ + /** + * Ctor. + * Uses a static lock to ensure that XSD files are written to the cache directory + * only once, even when multiple threads construct {`@link` StrictXmir} concurrently. + * `@param` before The XML source + * `@param` tmp The directory with cached XSD files + */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eo-parser/src/main/java/org/eolang/parser/StrictXmir.java` around lines 80 - 85, The Javadoc for the second StrictXmir constructor is stale: it says synchronization is done "by XML" but the code now uses Synced<>(Sticky<>(...)) for per-instance safety and a static LOCK for cross-instance file writes; update the comment on the constructor in class StrictXmir to describe the current mechanism (per-instance thread safety via Synced and Sticky wrappers and cross-instance file-write synchronization via the static LOCK) and remove the reference to locking on the XML object, mentioning the relevant symbols Synced, Sticky and LOCK and the XML parameter name to clarify behavior.
211-238:⚠️ Potential issue | 🟠 MajorGlobal static lock held during blocking I/O (disk write and network download)
LOCKis a single static instance shared across allStrictXmirinstances. Indownloaded, it is held for the entire duration of the network fetch — including up to three retry attempts, each of which can block for seconds. During that window, every other thread that calls eithercopiedordownloaded(even for a completely unrelated XSD file) is queued behind the same lock.The standard fix is a double-checked locking pattern around the I/O section. Because
File.exists()queries the OS filesystem (which provides its own cross-thread visibility guarantee), checking it outside the lock is safe — there is no partial-object publication hazard here (unlike Java object references).⚡ Proposed fix: double-checked locking in both methods
private static File copied(final String uri, final Path path) { final File file = path.toFile(); - StrictXmir.LOCK.lock(); - try { - if (!file.exists()) { + if (!file.exists()) { + StrictXmir.LOCK.lock(); + try { + if (!file.exists()) { if (file.getParentFile().mkdirs()) { Logger.debug(StrictXmir.class, "Directory for %[file]s created", path); } try { Files.write( path, new IoCheckedBytes( new BytesOf(new ResourceOf("XMIR.xsd")) ).asBytes() ); Logger.debug(StrictXmir.class, "XSD copied to %[file]s", path); } catch (final IOException ex) { throw new IllegalArgumentException( String.format("Failed to save %s to %s", uri, path), ex ); } + } + } finally { + StrictXmir.LOCK.unlock(); } - } finally { - StrictXmir.LOCK.unlock(); } return file; }private static File downloaded(final String uri, final Path path) { final File abs = path.toFile().getAbsoluteFile(); - StrictXmir.LOCK.lock(); - try { - if (!abs.exists()) { + if (!abs.exists()) { + StrictXmir.LOCK.lock(); + try { + if (!abs.exists()) { // ... directory creation, retry loop ... + } + } finally { + StrictXmir.LOCK.unlock(); } - } finally { - StrictXmir.LOCK.unlock(); } return abs; }After the first successful write, all subsequent callers skip the lock entirely on the fast-path
exists()check, eliminating contention for the common case.Also applies to: 247-298
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eo-parser/src/main/java/org/eolang/parser/StrictXmir.java` around lines 211 - 238, The code currently holds StrictXmir.LOCK for the entire blocking I/O operation in copied (and similarly in downloaded), which serializes unrelated threads; change both copied and downloaded to use double-checked locking: perform a fast-path check like if (file.exists()) return file before acquiring StrictXmir.LOCK, then inside the lock re-check file.exists() and only perform the write/download when it still does not exist; keep the existing try/finally that unlocks LOCK and preserve the existing exception handling and logging but ensure the lock only wraps the minimal critical section that creates/writes the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@eo-parser/src/main/java/org/eolang/parser/StrictXmir.java`:
- Around line 80-85: The Javadoc for the second StrictXmir constructor is stale:
it says synchronization is done "by XML" but the code now uses
Synced<>(Sticky<>(...)) for per-instance safety and a static LOCK for
cross-instance file writes; update the comment on the constructor in class
StrictXmir to describe the current mechanism (per-instance thread safety via
Synced and Sticky wrappers and cross-instance file-write synchronization via the
static LOCK) and remove the reference to locking on the XML object, mentioning
the relevant symbols Synced, Sticky and LOCK and the XML parameter name to
clarify behavior.
- Around line 211-238: The code currently holds StrictXmir.LOCK for the entire
blocking I/O operation in copied (and similarly in downloaded), which serializes
unrelated threads; change both copied and downloaded to use double-checked
locking: perform a fast-path check like if (file.exists()) return file before
acquiring StrictXmir.LOCK, then inside the lock re-check file.exists() and only
perform the write/download when it still does not exist; keep the existing
try/finally that unlocks LOCK and preserve the existing exception handling and
logging but ensure the lock only wraps the minimal critical section that
creates/writes the file.
|
@maxonfjvipon please check |
|
@yegor256 please check |
3 participants
see #4891
Summary by CodeRabbit