Fix assertion at end of forceRefreshes

[ywelsch]

I've had a local run on my CI where the assertion assert refreshListeners == null; at the end of forceRefreshes tripped during a relocation because a concurrent addOrNotify was called that set refreshListeners from null to an empty list.

This PR ensures, we only change refreshListeners to a list if we're actually adding something to the list.

[elasticmachine]

Pinging @elastic/es-distributed

[original-brownbear]

LGTM, one question/suggestion to package it differently maybe :)

[original-brownbear]

I wonder if it would be less confusing if we just moved the whole logic for creating the refreshListeners here

So:

diff --git a/server/src/main/java/org/elasticsearch/index/shard/RefreshListeners.java b/server/src/main/java/org/elasticsearch/index/shard/RefreshListeners.java
index b4b9e13f7e0..713563eb111 100644
--- a/server/src/main/java/org/elasticsearch/index/shard/RefreshListeners.java
+++ b/server/src/main/java/org/elasticsearch/index/shard/RefreshListeners.java
@@ -129,15 +129,12 @@ public final class RefreshListeners implements ReferenceManager.RefreshListener,
             return true;
         }
         synchronized (this) {
-            List<Tuple<Translog.Location, Consumer<Boolean>>> listeners = refreshListeners;
-            if (listeners == null) {
-                if (closed) {
-                    throw new IllegalStateException("can't wait for refresh on a closed index");
-                }
-                listeners = new ArrayList<>();
-                refreshListeners = listeners;
+            if (closed) {
+                throw new IllegalStateException("can't wait for refresh on a closed index");
             }
-            if (refreshForcers == 0 && listeners.size() < getMaxRefreshListeners.getAsInt()) {
+            List<Tuple<Translog.Location, Consumer<Boolean>>> listeners = refreshListeners;
+            final int maxRefreshes = getMaxRefreshListeners.getAsInt();
+            if (refreshForcers == 0 && maxRefreshes > 0 && (listeners == null || listeners.size() < maxRefreshes)) {
                 ThreadContext.StoredContext storedContext = threadContext.newStoredContext(true);
                 Consumer<Boolean> contextPreservingListener = forced -> {
                     try (ThreadContext.StoredContext ignore = threadContext.stashContext()) {
@@ -145,8 +142,12 @@ public final class RefreshListeners implements ReferenceManager.RefreshListener,
                         listener.accept(forced);
                     }
                 };
+                if (listeners == null) {
+                    listeners = new ArrayList<>();
+                }
                 // We have a free slot so register the listener
                 listeners.add(new Tuple<>(location, contextPreservingListener));
+                refreshListeners = listeners;
                 return false;
             }
         }

... would also save needlessly instantiating an ArrayList when refreshForcers is > 0?

[ywelsch]

sure, can you push that change directly to the PR?

[original-brownbear]

Done in ee15306

[ywelsch]

thanks

[original-brownbear]

Jenkins run Gradle build tests 1

[ywelsch]

@elasticmachine please run the Gradle build tests 1

[ywelsch]

@elasticmachine please run the Gradle build tests 1