From 6d58d80437617f94cf2d11bc5ab67cbaac21b8d9 Mon Sep 17 00:00:00 2001 From: Michael Simons Date: Mon, 21 Aug 2023 09:54:58 +0200 Subject: [PATCH] Fix synchronisation of bookmark access in reactive bookmark manager. The separate reactive bookmark manager was introduced because Project Reactor and Blockhound have issues with the `ReentrantReadWriteLock` approach we have taken in the imperative approach. The first approach was using a synchronized set, but as it was correctly noted by @seabamirum on #2769 this is not enough on the reading path: > It is imperative that the user manually synchronize on the returned set when iterating over it: (From the JavaDoc of `Collections.synchronizedSet`. Using `ConcurrentHashMap.newKeySet` would solve that issue, but it would require a check for `null` values in the `usedBookmarks` argument for `updateBookmarks` AND it would also not solve the fact that removing the used bookmarks and adding the new ones is an atomic operation (such as it originally was in the imperative world). So therefor it is just easier to use a standard set and synchronize over it. --- .../ReactiveDefaultBookmarkManager.java | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/springframework/data/neo4j/core/transaction/ReactiveDefaultBookmarkManager.java b/src/main/java/org/springframework/data/neo4j/core/transaction/ReactiveDefaultBookmarkManager.java index af74b78a1..7f6f732f8 100644 --- a/src/main/java/org/springframework/data/neo4j/core/transaction/ReactiveDefaultBookmarkManager.java +++ b/src/main/java/org/springframework/data/neo4j/core/transaction/ReactiveDefaultBookmarkManager.java @@ -36,7 +36,7 @@ import org.springframework.lang.Nullable; */ final class ReactiveDefaultBookmarkManager extends AbstractBookmarkManager { - private final Set bookmarks = Collections.synchronizedSet(new HashSet<>()); + private final Set bookmarks = new HashSet<>(); private final Supplier> bookmarksSupplier; @@ -49,14 +49,18 @@ final class ReactiveDefaultBookmarkManager extends AbstractBookmarkManager { @Override public Collection getBookmarks() { - this.bookmarks.addAll(bookmarksSupplier.get()); - return Set.copyOf(this.bookmarks); + synchronized (this.bookmarks) { + this.bookmarks.addAll(bookmarksSupplier.get()); + return Set.copyOf(this.bookmarks); + } } @Override public void updateBookmarks(Collection usedBookmarks, Collection newBookmarks) { - bookmarks.removeAll(usedBookmarks); - newBookmarks.stream().filter(Objects::nonNull).forEach(bookmarks::add); + synchronized (this.bookmarks) { + usedBookmarks.stream().filter(Objects::nonNull).forEach(bookmarks::remove); + newBookmarks.stream().filter(Objects::nonNull).forEach(bookmarks::add); + } if (applicationEventPublisher != null) { applicationEventPublisher.publishEvent(new Neo4jBookmarksUpdatedEvent(new HashSet<>(bookmarks))); }