Skip to content

Commit ba963db

Browse files
michael-simonsmeistermeier
authored andcommitted
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.
1 parent bc4fff0 commit ba963db

File tree

1 file changed

+9
-5
lines changed

1 file changed

+9
-5
lines changed

src/main/java/org/springframework/data/neo4j/core/transaction/ReactiveDefaultBookmarkManager.java

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@
3636
*/
3737
final class ReactiveDefaultBookmarkManager extends AbstractBookmarkManager {
3838

39-
private final Set<Bookmark> bookmarks = Collections.synchronizedSet(new HashSet<>());
39+
private final Set<Bookmark> bookmarks = new HashSet<>();
4040

4141
private final Supplier<Set<Bookmark>> bookmarksSupplier;
4242

@@ -49,14 +49,18 @@ final class ReactiveDefaultBookmarkManager extends AbstractBookmarkManager {
4949

5050
@Override
5151
public Collection<Bookmark> getBookmarks() {
52-
this.bookmarks.addAll(bookmarksSupplier.get());
53-
return Set.copyOf(this.bookmarks);
52+
synchronized (this.bookmarks) {
53+
this.bookmarks.addAll(bookmarksSupplier.get());
54+
return Set.copyOf(this.bookmarks);
55+
}
5456
}
5557

5658
@Override
5759
public void updateBookmarks(Collection<Bookmark> usedBookmarks, Collection<Bookmark> newBookmarks) {
58-
bookmarks.removeAll(usedBookmarks);
59-
newBookmarks.stream().filter(Objects::nonNull).forEach(bookmarks::add);
60+
synchronized (this.bookmarks) {
61+
usedBookmarks.stream().filter(Objects::nonNull).forEach(bookmarks::remove);
62+
newBookmarks.stream().filter(Objects::nonNull).forEach(bookmarks::add);
63+
}
6064
if (applicationEventPublisher != null) {
6165
applicationEventPublisher.publishEvent(new Neo4jBookmarksUpdatedEvent(new HashSet<>(bookmarks)));
6266
}

0 commit comments

Comments
 (0)