Open
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:
WalkthroughAdds debugging and observability to concurrent export tests: timestamped logging, thread-dump utilities, a JUnit watchdog rule, per-download string IDs, and verbose lifecycle logging. Updates ConcurrentStreamResourceWriter to require a String id and threads are named per-download. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![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
🧹 Nitpick comments (1)
src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java (1)
71-75: Make the elapsed-time base per test.
testStartTimeis initialized once when the class loads, so later methods log time accumulated from earlier tests. That makes the watchdog output harder to correlate with the current failure. Reset the start time at the beginning of each test instead of keeping it static.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java` around lines 71 - 75, The elapsed-time base is currently a static final testStartTime set at class load, so timestamp() reports time since class load rather than per-test; change testStartTime to a non-static (remove static and final) instance field and add a JUnit `@Before` method (e.g. setUp or resetTestStartTime) that sets testStartTime = System.currentTimeMillis() before each test, ensuring timestamp() (which should reference the instance field) reports elapsed time relative to the start of the current test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java`:
- Around line 225-227: The new diagnostic log in the
ConcurrentStreamResourceWriter delegate can lengthen setup before
writer.accept() and latch.countDown(), causing start() (which still only waits
1ms) to proceed before q2/q3 are actually contending; to fix, add a separate
"readyToContend" CountDownLatch (or reuse a new latch) that you count down
immediately before invoking writer.accept(), have start() wait on that
readyToContend latch instead of proceeding after 1ms, and/or move the diagnostic
log lines after the synchronization point so that start(), writer.accept(),
latch.countDown(), and the tests like testInterruptedByTimeout3 see the threads
actually ready to contend for permits (update references in
ConcurrentStreamResourceWriter delegate, start(), and the test code around
q2/q3).
---
Nitpick comments:
In
`@src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java`:
- Around line 71-75: The elapsed-time base is currently a static final
testStartTime set at class load, so timestamp() reports time since class load
rather than per-test; change testStartTime to a non-static (remove static and
final) instance field and add a JUnit `@Before` method (e.g. setUp or
resetTestStartTime) that sets testStartTime = System.currentTimeMillis() before
each test, ensuring timestamp() (which should reference the instance field)
reports elapsed time relative to the start of the current test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aaaec031-6a18-4816-9157-241e48fb0632
📒 Files selected for processing (1)
src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java
src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java
Show resolved
Hide resolved
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
`@src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java`:
- Line 71: Rename the static final field testStartTime to follow Java constant
naming conventions (e.g., TEST_START_TIME) in the ConcurrentExportTests class;
update the declaration of the constant (preserving its value
System.currentTimeMillis()) and replace all references/usages of testStartTime
throughout the class to the new name (TEST_START_TIME) to ensure compilation and
consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b71cc6d2-4ddb-4262-87d6-9be38695aa77
📒 Files selected for processing (1)
src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java
src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java (2)
70-74:⚠️ Potential issue | 🟡 MinorRename
testStartTimeto UPPER_SNAKE_CASE to satisfy static analysis.
static finalconstant naming is still non-compliant.Proposed rename
- private static final long testStartTime = System.currentTimeMillis(); + private static final long TEST_START_TIME = System.currentTimeMillis(); @@ - return String.format("[t=%5dms]", System.currentTimeMillis() - testStartTime); + return String.format("[t=%5dms]", System.currentTimeMillis() - TEST_START_TIME);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java` around lines 70 - 74, Rename the static final field testStartTime to UPPER_SNAKE_CASE (e.g., TEST_START_TIME) to satisfy static analysis and update all references (notably the timestamp() method which uses testStartTime) to use the new constant name; ensure the field remains static final long and the timestamp() formatting logic is unchanged.
214-215:⚠️ Potential issue | 🔴 CriticalFix remaining startup race in
start(); it still returns before contention is guaranteed.The 1ms wait in
start()is still nondeterministic, and the extra pre-sync logging increases the chance of returning too early. This is consistent with the flaky behavior seen at Line 498 (expected <0> but was <1>).
Use a dedicated “ready to contend” latch and wait on it instead of timing-based sleep/await.Proposed stabilization patch
@@ private MockDownload newDownload(String id) { @@ CyclicBarrier barrier = this.barrier; CountDownLatch latch = new CountDownLatch(1); + CountDownLatch readyToContend = new CountDownLatch(1); @@ ConcurrentStreamResourceWriter writer = new ConcurrentStreamResourceWriter(id, (stream, session) -> { - log("download-" + id + ": delegate running — counting down latch"); latch.countDown(); + log("download-" + id + ": delegate running"); log("download-" + id + ": delegate entering barrier (waiting=" + barrier.getNumberWaiting() + ")"); await(barrier); log("download-" + id + ": delegate exited barrier"); }); @@ Thread thread = newThread(() -> { @@ Throwable throwable = null; try { + readyToContend.countDown(); writer.accept(NULL_OUTPUT_STREAM, createSession()); } catch (Throwable t) { @@ `@Override` public MockDownload start() { if (thread.getState() == Thread.State.NEW) { thread.start(); } try { - latch.await(1, TimeUnit.MILLISECONDS); + if (!readyToContend.await(TEST_TIMEOUT, TimeUnit.MILLISECONDS)) { + throw new IllegalStateException("Download thread did not reach contention point"); + } } catch (InterruptedException e) { sneakyThrow(e); } return this; }Also applies to: 266-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java` around lines 214 - 215, The start() method has a race because it uses a 1ms sleep to wait for threads to be ready; replace that timing-based wait with a dedicated CountDownLatch (e.g., readyToContend) so start() blocks until all workers signal they are ready to contend; add a new CountDownLatch readyToContend initialized to the expected thread count, have each worker/delegate call readyToContend.countDown() at the point currently logging "delegate running — counting down latch" (and at the analogous spot around lines 266-268), and change start() to await readyToContend.await(...) before returning so contention is deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.java`:
- Around line 70-74: Rename the static final field testStartTime to
UPPER_SNAKE_CASE (e.g., TEST_START_TIME) to satisfy static analysis and update
all references (notably the timestamp() method which uses testStartTime) to use
the new constant name; ensure the field remains static final long and the
timestamp() formatting logic is unchanged.
- Around line 214-215: The start() method has a race because it uses a 1ms sleep
to wait for threads to be ready; replace that timing-based wait with a dedicated
CountDownLatch (e.g., readyToContend) so start() blocks until all workers signal
they are ready to contend; add a new CountDownLatch readyToContend initialized
to the expected thread count, have each worker/delegate call
readyToContend.countDown() at the point currently logging "delegate running —
counting down latch" (and at the analogous spot around lines 266-268), and
change start() to await readyToContend.await(...) before returning so contention
is deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a44f5f1f-ef58-44ee-9464-e8c7c0466ab7
📒 Files selected for processing (1)
src/test/java/com/flowingcode/vaadin/addons/gridexporter/test/ConcurrentExportTests.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
1 participant
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 by CodeRabbit