Skip to content

Comments

async pathfinding: fix path race condition#620

Open
hayanesuru wants to merge 1 commit intoWinds-Studio:ver/1.21.11from
hayanesuru:p/42-path-race
Open

async pathfinding: fix path race condition#620
hayanesuru wants to merge 1 commit intoWinds-Studio:ver/1.21.11from
hayanesuru:p/42-path-race

Conversation

@hayanesuru
Copy link
Collaborator

@hayanesuru hayanesuru commented Feb 2, 2026

#519 #589

Closed #604

@hayanesuru hayanesuru force-pushed the p/42-path-race branch 3 times, most recently from c646459 to f317d41 Compare February 8, 2026 17:32
Copy link
Collaborator

@MartijnMuijsers MartijnMuijsers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

* ConcurrentLinkedQueue is thread-safe, non-blocking and non-synchronized
*/
private final ConcurrentLinkedQueue<Runnable> postProcessing = new ConcurrentLinkedQueue<>();
private final ArrayList<Consumer<Path>> postProcessing = new ArrayList<>();
Copy link
Collaborator

@MartijnMuijsers MartijnMuijsers Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good change!

Copy link
Collaborator

@MartijnMuijsers MartijnMuijsers Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming schedulePostProcessing is only called from the main thread, correct? Otherwise the .add will cause issues when called from multiple threads

if (!ready) {
Path ret = this.ret;
if (ret != null) {
complete(ret);
Copy link
Collaborator

@MartijnMuijsers MartijnMuijsers Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this path is only completed upon isDone, what happens upon isProcessed? Shoud it also call complete?

I assume isDone() and isProcessed() can only be run from the main thread, right?

}

return this.positions.containsAll(positions);
return this.positions.equals(positions);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original is containsAll, this changes actual behavior

@@ -67,7 +67,7 @@ protected static CompletableFuture<Void> queue(AsyncPath path) {
*/
public static void awaitProcessing(@Nullable Path path, Consumer<@Nullable Path> afterProcessing) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This already existed in the original, but the name await here is not really correct. applyAfterProcessing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Entity threw exception (ConcurrentModificationException). Async pathfinding bug?

2 participants