Improve shutdown logic for ClientFeederV3

Remove call to Object.wait(long). This call would always fail since
there was no monitor lock on the object being waited on.
The 'kill()' method should now longer throw exception and halt kill of
subsequent client feeder instances.

2 files changed, 21 insertions, 19 deletions

diff --git a/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/ClientFeederV3.java b/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/ClientFeederV3.java
index d57ef11f2d3..66955be2325 100644
--- a/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/ClientFeederV3.java
+++ b/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/ClientFeederV3.java
@@ -8,6 +8,7 @@ import com.yahoo.documentapi.messagebus.protocol.DocumentMessage;
 import com.yahoo.documentapi.messagebus.protocol.DocumentProtocol;
 import com.yahoo.jdisc.Metric;
 import com.yahoo.jdisc.ReferencedResource;
+import com.yahoo.jdisc.ResourceReference;
 import com.yahoo.messagebus.Message;
 import com.yahoo.messagebus.ReplyHandler;
 import com.yahoo.messagebus.Result;
@@ -71,22 +72,25 @@ class ClientFeederV3 {
         this.hostName = HostName.getLocalhost();
     }
 
-    public boolean timedOut() {
+    boolean timedOut() {
         synchronized (monitor) {
             return Instant.now().isAfter(prevOpsPerSecTime.plusSeconds(6000)) && ongoingRequests.get() == 0;
         }
     }
 
-    public void kill() {
-        // No new requests should be sent to this object, but there can be old one, even though this is very unlikely.
-        while (ongoingRequests.get() > 0) {
-            try {
-                ongoingRequests.wait(100);
-            } catch (InterruptedException e) {
-                break;
+    void kill() {
+        try (ResourceReference ignored = sourceSession.getReference()) {
+            // No new requests should be sent to this object, but there can be old one, even though this is very unlikely.
+            while (ongoingRequests.get() > 0) {
+                try {
+                    Thread.sleep(100);
+                } catch (InterruptedException e) {
+                    return;
+                }
             }
+        } catch (Exception e) {
+            log.log(Level.WARNING, "Failed to close reference to source session", e);
         }
-        sourceSession.getReference().close();
     }
 
     private void transferPreviousRepliesToResponse(BlockingQueue<OperationStatus> operations) throws InterruptedException {
@@ -98,7 +102,7 @@ class ClientFeederV3 {
         }
     }
 
-    public HttpResponse handleRequest(HttpRequest request) throws IOException {
+    HttpResponse handleRequest(HttpRequest request) throws IOException {
         ongoingRequests.incrementAndGet();
         try {
             FeederSettings feederSettings = new FeederSettings(request);
diff --git a/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/FeedHandlerV3.java b/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/FeedHandlerV3.java
index 620c8400434..5de4f5cbb14 100644
--- a/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/FeedHandlerV3.java
+++ b/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/FeedHandlerV3.java
@@ -18,7 +18,6 @@ import com.yahoo.vespa.http.client.core.Headers;
 import com.yahoo.yolean.Exceptions;
 
 import java.util.HashMap;
-import java.util.Iterator;
 import java.util.Map;
 import java.util.concurrent.Executor;
 import java.util.concurrent.ScheduledThreadPoolExecutor;
@@ -101,19 +100,19 @@ public class FeedHandlerV3 extends ThreadedHttpRequestHandler {
 
     @Override
     protected void destroy() {
-        // We are forking this to avoid that accidental dereferrencing causes any random thread doing destruction.
+        // We are forking this to avoid that accidental de-referencing causes any random thread doing destruction.
         // This caused a deadlock when the single Messenger thread in MessageBus was the last one referring this
         // and started destructing something that required something only the messenger thread could provide.
         Thread destroyer = new Thread(() -> {
             super.destroy();
             cron.shutdown();
             synchronized (monitor) {
-                for (ClientFeederV3 client : clientFeederByClientId.values()) {
-                    client.kill();
+                for (var iterator = clientFeederByClientId.values().iterator(); iterator.hasNext(); ) {
+                    iterator.next().kill();
+                    iterator.remove();
                 }
-                clientFeederByClientId.clear();
             }
-        });
+        }, "feed-handler-v3-adhoc-destroyer");
         destroyer.setDaemon(true);
         destroyer.start();
     }
@@ -142,9 +141,8 @@ public class FeedHandlerV3 extends ThreadedHttpRequestHandler {
 
     private void removeOldClients() {
         synchronized (monitor) {
-            for (Iterator<Map.Entry<String, ClientFeederV3>> iterator = clientFeederByClientId
-                    .entrySet().iterator(); iterator.hasNext();) {
-                ClientFeederV3 client = iterator.next().getValue();
+            for (var iterator = clientFeederByClientId.values().iterator(); iterator.hasNext(); ) {
+                ClientFeederV3 client = iterator.next();
                 if (client.timedOut()) {
                     client.kill();
                     iterator.remove();