Fixing race condition between end-operation and future completion. #7705
Conversation
sdedic
added
Gradle
|
Still does not work ;) so I'll play with it ... will ping after it is reviewable. |
|
@mbien seems better now. The 2nd failure was actually a bad test diagnostics ... but while fixing I made sure that the events created during a reload are delivered before next reload starts. |
|
@mbien ping |
There was a problem hiding this comment.
I ran maven and gradle tests 20 times in sequence using a custom workflow on my clone and got only one failure in the gradle step
https://github.com/mbien/netbeans/actions/runs/10656862483 gradle runs
https://github.com/mbien/netbeans/actions/runs/10659082827 maven runs (still running but it looks good so far)
so the fix seems to work I believe. Some comments inline.
2024-09-01T20:57:07.1157732Z [junit] Testcase: testGeneratedSources(org.netbeans.modules.gradle.java.classpath.GradleSourcesImplTest): FAILED
2024-09-01T20:57:07.1159927Z [junit] expected:<2> but was:<0>
2024-09-01T20:57:07.1160727Z [junit] junit.framework.AssertionFailedError: expected:<2> but was:<0>
2024-09-01T20:57:07.1162431Z [junit] at org.netbeans.modules.gradle.java.classpath.GradleSourcesImplTest.testGeneratedSources(GradleSourcesImplTest.java:55)
2024-09-01T20:57:07.1164429Z [junit] at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
2024-09-01T20:57:07.1166016Z [junit] at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
2024-09-01T20:57:07.1167834Z [junit] at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
2024-09-01T20:57:07.1169325Z [junit] at org.netbeans.junit.NbTestCase.access$200(NbTestCase.java:83)
2024-09-01T20:57:07.1170432Z [junit] at org.netbeans.junit.NbTestCase$2.doSomething(NbTestCase.java:488)
2024-09-01T20:57:07.1171555Z [junit] at org.netbeans.junit.NbTestCase$1Guard.run(NbTestCase.java:409)
2024-09-01T20:57:07.1172530Z [junit] at java.base/java.lang.Thread.run(Thread.java:840)
...ect.dependency/src/org/netbeans/modules/project/dependency/reload/ProjectReloadInternal.java
Outdated
Show resolved
Hide resolved
| CompletableFuture<ProjectState> f = CompletableFuture.runAsync(() -> nextReloader.initRound(), loader). | ||
| thenCompose((v) -> nextReloader.start(floader)); | ||
| f.whenComplete((a, b) -> { | ||
| CompletableFuture<ProjectState> f = CompletableFuture.runAsync(() -> fNextReloader.initRound(), loader). | ||
| thenCompose((v) -> fNextReloader.start(floader)); | ||
| // run this cleanup in the dispatcher thread | ||
| f.whenCompleteAsync((a, b) -> { |
There was a problem hiding this comment.
a is the state and b an exception. Please give things names, this makes it more difficult to review on github than necessary.
simplified the block a bit
CompletableFuture.runAsync(() -> fNextReloader.initRound(), loader)
.thenCompose((v) -> fNextReloader.start(floader))
.whenCompleteAsync((projectState, throwable) -> { // run this cleanup in the dispatcher threadThere was a problem hiding this comment.
Understood, but separating the 1st part into a temporary turned out to be much nicer when debugging ;)
...ect.dependency/src/org/netbeans/modules/project/dependency/reload/ProjectReloadInternal.java
Outdated
Show resolved
Hide resolved
...ect.dependency/src/org/netbeans/modules/project/dependency/reload/ProjectReloadInternal.java
Outdated
Show resolved
Hide resolved
|
I pressed the wrong button, wanted to comment and not request changes - most of it are just suggestions. |
|
I suppose we can link #7263 to this PR too? |
sdedic
force-pushed
the
project/fix-tests-endoperation
branch
from
b80cde2 to
9dcefae
Compare
|
rebased / squashed. |
| private Collection<IdentityHolder> collectReleases(ProjectOperations op) { | ||
| Collection<IdentityHolder> releases = op.releases; | ||
| // this is copied from postCleanup, but can be done in batch without | ||
| // checking the project operation is in progress for each reference. These are already removed | ||
| // from stateIdentity, so just check they did not obtain another one: | ||
| releases.removeIf(expired -> { | ||
| ProjectStateData d = expired.state.get(); | ||
| if (d == null) { | ||
| return true; | ||
| } | ||
| IdentityHolder h = stateIdentity.get(d); | ||
| return h != null && h != expired; | ||
| }); | ||
| op.releases = new ArrayList<>(); |
There was a problem hiding this comment.
moving L777 to L766 would make this a little bit safer.
There was a problem hiding this comment.
IMHO not important as entire call executes under synchronized(this) which also guards other accesses to op.releases. Maybe optically safer = adjacent to taking out the value.
@mbien mentioned test failures in #7655, I've experienced them too in #7665 (action run) and #7697 (action run).
I've "sort of" traced/guessed it to finalization of a reload: in the original code, the Future completes (which means it unblocks the test thread), then the reload formally ends (pending reload is removed). The test may be able to observe the still-running reload (nearing its end) during its
tearDown. The order was chosen so that events are fired only after Future completion, so potential observers see the new state as completed/ready.The PR splits the "formal termination" of a pending reload from event delivery, so the sequence is now:
This sequence is actually better / more predictable for clients.