You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@netbeans.apache.org by "sdedic (via GitHub)" <gi...@apache.org> on 2024/08/29 07:50:13 UTC
[PR] Fixing race condition between end-operation and future completion. [netbeans]
sdedic opened a new pull request, #7705:
URL: https://github.com/apache/netbeans/pull/7705
@mbien mentioned test failures in #7655, I've experienced them too in #7665 ([action run](https://github.com/apache/netbeans/actions/runs/10598938114/attempts/1)) and #7697 ([action run](https://github.com/apache/netbeans/actions/runs/10595857319/attempts/1)).
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:
- end the reload, potentially prepare the next one in the queue
- complete the reload Future
- deliver events and proceed with the next reload
This sequence is actually better / more predictable for clients.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing race condition between end-operation and future completion. [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #7705:
URL: https://github.com/apache/netbeans/pull/7705#discussion_r1740222524
##########
ide/project.dependency/src/org/netbeans/modules/project/dependency/reload/ProjectReloadInternal.java:
##########
@@ -843,32 +891,34 @@ private void endOperation(Project p, Reloader reload) {
} catch (InterruptedException ex) {
}
}
-
+
RequestProcessor floader = loader;
- 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) -> {
Review 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
```java
CompletableFuture.runAsync(() -> fNextReloader.initRound(), loader)
.thenCompose((v) -> fNextReloader.start(floader))
.whenCompleteAsync((projectState, throwable) -> { // run this cleanup in the dispatcher thread
```
##########
ide/project.dependency/src/org/netbeans/modules/project/dependency/reload/ProjectReloadInternal.java:
##########
@@ -743,32 +744,64 @@ public void runProjectAction(Project p, Runnable r) {
public void assertNoOperations() {
synchronized (this) {
- if (!pendingOperations.isEmpty()) {
+ Map<Project, ProjectOperations> ops = new HashMap<>(this.pendingOperations);
+ ops.values().removeAll(this.terminatingOperations);
+ if (!ops.isEmpty()) {
System.err.println("Pending operations detected");
- for (Map.Entry<Project, ProjectOperations> en : pendingOperations.entrySet()) {
+ for (Map.Entry<Project, ProjectOperations> en : ops.entrySet()) {
ProjectOperations op = en.getValue();
System.err.println(en.getKey() + ": usage " + op.usage + ", pendingReloads: " + op.pendingReloads.size() + ", actions: " + op.postponedActions.size());
for (Reloader r : op.pendingReloads) {
r.getOriginTrace().printStackTrace();
}
}
}
- if (!pendingOperations.isEmpty() || this.loaderProcessors.size() != PROJECT_RELOAD_CONCURRENCY) {
+ if (!ops.isEmpty() || this.loaderProcessors.size() != PROJECT_RELOAD_CONCURRENCY) {
throw new IllegalStateException();
}
}
}
+ private Collection<IdentityHolder> collectRelaeases(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:
+ for (Iterator<IdentityHolder> it = releases.iterator(); it.hasNext(); ) {
+ IdentityHolder expired = it.next();
+ ProjectStateData d = expired.state.get();
+ if (d == null) {
+ it.remove();
+ }
+ IdentityHolder h = stateIdentity.get(d);
+ if (h != null && h != expired) {
+ it.remove();
+ }
+ }
+ return releases;
+ }
Review Comment:
Relaeases -> Releases
this copies the releases list and simplifies a few things:
```java
private Collection<IdentityHolder> collectReleases(ProjectOperations op) {
List<IdentityHolder> releases = new ArrayList<>(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;
});
return releases;
}
```
alternative don't copy the list but also return nothing, to make it clear that the update is in-place
##########
ide/project.dependency/src/org/netbeans/modules/project/dependency/reload/ProjectReloadInternal.java:
##########
@@ -843,32 +891,34 @@ private void endOperation(Project p, Reloader reload) {
} catch (InterruptedException ex) {
}
}
-
+
RequestProcessor floader = loader;
- 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) -> {
+ LOG.log(Level.FINER, "Return RP to the pool", new Object[] { floader });
+ loaderProcessors.offer(floader);
try {
- LOG.log(Level.FINE, "Load end project {0} with request {1}", new Object[] { nextReloader.project, nextReloader.request });
- if (b == null) {
- nextReloader.completePending.completeAsync(() -> a, RELOAD_RP);
- } else {
- RELOAD_RP.post(() -> {
- nextReloader.completePending.completeExceptionally(b);
- });
- }
- // this will eventually fire postponed events.
- endOperation(nextReloader.project, nextReloader);
- LOG.log(Level.FINER, "Return RP to the pool", new Object[] { floader });
+ LOG.log(Level.FINE, "Load end project {0} with request {1}", new Object[] { fNextReloader.project, fNextReloader.request });
+ // postpone event delivery so that the events observers see the Future as completed.
+ endOperation(fNextReloader.project, fNextReloader, () -> {;
+ if (b == null) {
+ fNextReloader.completePending.completeAsync(() -> a, RELOAD_RP);
+ } else {
+ RELOAD_RP.post(() -> {
+ fNextReloader.completePending.completeExceptionally(b);
+ });
+ }
+ });
} catch (ThreadDeath td) {
throw td;
} catch (Throwable t) {
Exceptions.printStackTrace(t);
} finally {
- loaderProcessors.offer(floader);
}
Review Comment:
finally block can now be removed
##########
ide/project.dependency/src/org/netbeans/modules/project/dependency/reload/ProjectReloadInternal.java:
##########
@@ -843,32 +891,34 @@ private void endOperation(Project p, Reloader reload) {
} catch (InterruptedException ex) {
}
}
-
+
RequestProcessor floader = loader;
- 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) -> {
+ LOG.log(Level.FINER, "Return RP to the pool", new Object[] { floader });
+ loaderProcessors.offer(floader);
try {
- LOG.log(Level.FINE, "Load end project {0} with request {1}", new Object[] { nextReloader.project, nextReloader.request });
- if (b == null) {
- nextReloader.completePending.completeAsync(() -> a, RELOAD_RP);
- } else {
- RELOAD_RP.post(() -> {
- nextReloader.completePending.completeExceptionally(b);
- });
- }
- // this will eventually fire postponed events.
- endOperation(nextReloader.project, nextReloader);
- LOG.log(Level.FINER, "Return RP to the pool", new Object[] { floader });
+ LOG.log(Level.FINE, "Load end project {0} with request {1}", new Object[] { fNextReloader.project, fNextReloader.request });
+ // postpone event delivery so that the events observers see the Future as completed.
+ endOperation(fNextReloader.project, fNextReloader, () -> {;
Review Comment:
redundant semicolon
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing race condition between end-operation and future completion. [netbeans]
Posted by "sdedic (via GitHub)" <gi...@apache.org>.
sdedic merged PR #7705:
URL: https://github.com/apache/netbeans/pull/7705
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing race condition between end-operation and future completion. [netbeans]
Posted by "sdedic (via GitHub)" <gi...@apache.org>.
sdedic commented on PR #7705:
URL: https://github.com/apache/netbeans/pull/7705#issuecomment-2318668815
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing race condition between end-operation and future completion. [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on PR #7705:
URL: https://github.com/apache/netbeans/pull/7705#issuecomment-2323649657
I suppose we can link https://github.com/apache/netbeans/issues/7263 to this PR too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing race condition between end-operation and future completion. [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #7705:
URL: https://github.com/apache/netbeans/pull/7705#discussion_r1741074873
##########
ide/project.dependency/src/org/netbeans/modules/project/dependency/reload/ProjectReloadInternal.java:
##########
@@ -743,32 +743,62 @@ public void runProjectAction(Project p, Runnable r) {
public void assertNoOperations() {
synchronized (this) {
- if (!pendingOperations.isEmpty()) {
+ Map<Project, ProjectOperations> ops = new HashMap<>(this.pendingOperations);
+ ops.values().removeAll(this.terminatingOperations);
+ if (!ops.isEmpty()) {
System.err.println("Pending operations detected");
- for (Map.Entry<Project, ProjectOperations> en : pendingOperations.entrySet()) {
+ for (Map.Entry<Project, ProjectOperations> en : ops.entrySet()) {
ProjectOperations op = en.getValue();
System.err.println(en.getKey() + ": usage " + op.usage + ", pendingReloads: " + op.pendingReloads.size() + ", actions: " + op.postponedActions.size());
for (Reloader r : op.pendingReloads) {
r.getOriginTrace().printStackTrace();
}
}
}
- if (!pendingOperations.isEmpty() || this.loaderProcessors.size() != PROJECT_RELOAD_CONCURRENCY) {
+ if (!ops.isEmpty() || this.loaderProcessors.size() != PROJECT_RELOAD_CONCURRENCY) {
throw new IllegalStateException();
}
}
}
+ 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<>();
Review Comment:
moving L777 to L766 would make this a little bit safer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing race condition between end-operation and future completion. [netbeans]
Posted by "sdedic (via GitHub)" <gi...@apache.org>.
sdedic commented on PR #7705:
URL: https://github.com/apache/netbeans/pull/7705#issuecomment-2324732428
rebased / squashed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing race condition between end-operation and future completion. [netbeans]
Posted by "sdedic (via GitHub)" <gi...@apache.org>.
sdedic commented on PR #7705:
URL: https://github.com/apache/netbeans/pull/7705#issuecomment-2317080964
Still does not work ;) so I'll play with it ... will ping after it is reviewable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing race condition between end-operation and future completion. [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on PR #7705:
URL: https://github.com/apache/netbeans/pull/7705#issuecomment-2323640505
I pressed the wrong button, wanted to comment and not request changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing race condition between end-operation and future completion. [netbeans]
Posted by "sdedic (via GitHub)" <gi...@apache.org>.
sdedic commented on code in PR #7705:
URL: https://github.com/apache/netbeans/pull/7705#discussion_r1741224058
##########
ide/project.dependency/src/org/netbeans/modules/project/dependency/reload/ProjectReloadInternal.java:
##########
@@ -743,32 +743,62 @@ public void runProjectAction(Project p, Runnable r) {
public void assertNoOperations() {
synchronized (this) {
- if (!pendingOperations.isEmpty()) {
+ Map<Project, ProjectOperations> ops = new HashMap<>(this.pendingOperations);
+ ops.values().removeAll(this.terminatingOperations);
+ if (!ops.isEmpty()) {
System.err.println("Pending operations detected");
- for (Map.Entry<Project, ProjectOperations> en : pendingOperations.entrySet()) {
+ for (Map.Entry<Project, ProjectOperations> en : ops.entrySet()) {
ProjectOperations op = en.getValue();
System.err.println(en.getKey() + ": usage " + op.usage + ", pendingReloads: " + op.pendingReloads.size() + ", actions: " + op.postponedActions.size());
for (Reloader r : op.pendingReloads) {
r.getOriginTrace().printStackTrace();
}
}
}
- if (!pendingOperations.isEmpty() || this.loaderProcessors.size() != PROJECT_RELOAD_CONCURRENCY) {
+ if (!ops.isEmpty() || this.loaderProcessors.size() != PROJECT_RELOAD_CONCURRENCY) {
throw new IllegalStateException();
}
}
}
+ 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<>();
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing race condition between end-operation and future completion. [netbeans]
Posted by "sdedic (via GitHub)" <gi...@apache.org>.
sdedic commented on PR #7705:
URL: https://github.com/apache/netbeans/pull/7705#issuecomment-2323431156
@mbien ping
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Fixing race condition between end-operation and future completion. [netbeans]
Posted by "sdedic (via GitHub)" <gi...@apache.org>.
sdedic commented on code in PR #7705:
URL: https://github.com/apache/netbeans/pull/7705#discussion_r1740691492
##########
ide/project.dependency/src/org/netbeans/modules/project/dependency/reload/ProjectReloadInternal.java:
##########
@@ -843,32 +891,34 @@ private void endOperation(Project p, Reloader reload) {
} catch (InterruptedException ex) {
}
}
-
+
RequestProcessor floader = loader;
- 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) -> {
Review Comment:
Understood, but separating the 1st part into a temporary turned out to be much nicer when debugging ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists