Make sure we don't swallow exceptions when transferring a file fails

4 files changed, 33 insertions, 83 deletions

diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileServer.java b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileServer.java
index 57d57d16d2f..f4722a9e7cc 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileServer.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/filedistribution/FileServer.java
@@ -12,7 +12,6 @@ import com.yahoo.jrt.StringValue;
 import com.yahoo.jrt.Supervisor;
 import com.yahoo.jrt.Transport;
 import com.yahoo.vespa.config.ConnectionPool;
-import com.yahoo.vespa.filedistribution.EmptyFileReferenceData;
 import com.yahoo.vespa.filedistribution.FileDistributionConnectionPool;
 import com.yahoo.vespa.filedistribution.FileDownloader;
 import com.yahoo.vespa.filedistribution.FileReferenceCompressor;
@@ -20,7 +19,6 @@ import com.yahoo.vespa.filedistribution.FileReferenceData;
 import com.yahoo.vespa.filedistribution.FileReferenceDownload;
 import com.yahoo.vespa.filedistribution.LazyFileReferenceData;
 import com.yahoo.vespa.filedistribution.LazyTemporaryStorageFileReferenceData;
-import com.yahoo.yolean.Exceptions;
 import java.io.File;
 import java.io.IOException;
 import java.nio.file.Files;
@@ -35,10 +33,15 @@ import java.util.logging.Level;
 import java.util.logging.Logger;
 
 import static com.yahoo.vespa.config.server.filedistribution.FileDistributionUtil.getOtherConfigServersInCluster;
+import static com.yahoo.vespa.config.server.filedistribution.FileServer.FileApiErrorCodes.NOT_FOUND;
+import static com.yahoo.vespa.config.server.filedistribution.FileServer.FileApiErrorCodes.OK;
+import static com.yahoo.vespa.config.server.filedistribution.FileServer.FileApiErrorCodes.TIMEOUT;
+import static com.yahoo.vespa.config.server.filedistribution.FileServer.FileApiErrorCodes.TRANSFER_FAILED;
 import static com.yahoo.vespa.filedistribution.FileReferenceData.CompressionType;
 import static com.yahoo.vespa.filedistribution.FileReferenceData.CompressionType.gzip;
 import static com.yahoo.vespa.filedistribution.FileReferenceData.Type;
 import static com.yahoo.vespa.filedistribution.FileReferenceData.Type.compressed;
+import static com.yahoo.yolean.Exceptions.uncheck;
 
 public class FileServer {
 
@@ -54,10 +57,11 @@ public class FileServer {
     private final List<CompressionType> compressionTypes; // compression types to use, in preferred order
 
     // TODO: Move to filedistribution module, so that it can be used by both clients and servers
-    private enum FileApiErrorCodes {
+    enum FileApiErrorCodes {
         OK(0, "OK"),
         NOT_FOUND(1, "File reference not found"),
-        TIMEOUT(2, "Timeout");
+        TIMEOUT(2, "Timeout"),
+        TRANSFER_FAILED(3, "Failed transferring file");
         private final int code;
         private final String description;
         FileApiErrorCodes(int code, String description) {
@@ -114,29 +118,28 @@ public class FileServer {
     FileDirectory getRootDir() { return fileDirectory; }
 
     void startFileServing(FileReference reference, Receiver target, Set<CompressionType> acceptedCompressionTypes) {
-        if ( ! fileDirectory.getFile(reference).exists()) return;
+        File file = fileDirectory.getFile(reference);
+        if ( ! file.exists()) return;
+
+        FileReferenceData fileData =
+                uncheck(() -> fileReferenceData(reference, acceptedCompressionTypes, file),
+                        "For " + reference.value() + ": failed reading file '" + file.getAbsolutePath() + "'" +
+                                " for sending to '" + target.toString() + "'. ");
 
-        File file = this.fileDirectory.getFile(reference);
-        log.log(Level.FINE, () -> "Start serving " + reference + " with file '" + file.getAbsolutePath() + "'");
-        FileReferenceData fileData = EmptyFileReferenceData.empty(reference, file.getName());
         try {
-            fileData = readFileReferenceData(reference, acceptedCompressionTypes);
+            log.log(Level.FINE, () -> "Start serving " + reference.value() + " with file '" + file.getAbsolutePath() + "'");
             target.receive(fileData, new ReplayStatus(0, "OK"));
             log.log(Level.FINE, () -> "Done serving " + reference.value() + " with file '" + file.getAbsolutePath() + "'");
-        } catch (IOException e) {
-            String errorDescription = "For" + reference.value() + ": failed reading file '" + file.getAbsolutePath() + "'";
-            log.warning(errorDescription + " for sending to '" + target.toString() + "'. " + e.getMessage());
-            target.receive(fileData, new ReplayStatus(1, errorDescription));
         } catch (Exception e) {
-            log.log(Level.WARNING, "Failed serving " + reference + ": " + Exceptions.toMessageString(e));
+            throw new RuntimeException("Failed serving " + reference.value() + " to '" + target + "': ", e);
         } finally {
             fileData.close();
         }
     }
 
-    private FileReferenceData readFileReferenceData(FileReference reference, Set<CompressionType> acceptedCompressionTypes) throws IOException {
-        File file = this.fileDirectory.getFile(reference);
-
+    private FileReferenceData fileReferenceData(FileReference reference,
+                                                Set<CompressionType> acceptedCompressionTypes,
+                                                File file) throws IOException {
         if (file.isDirectory()) {
             Path tempFile = Files.createTempFile("filereferencedata", reference.value());
             CompressionType compressionType = chooseCompressionType(acceptedCompressionTypes);
@@ -172,20 +175,21 @@ public class FileServer {
                                                 Set<CompressionType> acceptedCompressionTypes) {
         if (Instant.now().isAfter(deadline)) {
             log.log(Level.INFO, () -> "Deadline exceeded for request for file reference '" + fileReference + "' from " + client);
-            return FileApiErrorCodes.TIMEOUT;
+            return TIMEOUT;
         }
 
-        boolean fileExists;
         try {
             var fileReferenceDownload = new FileReferenceDownload(fileReference, client, downloadFromOtherSourceIfNotFound);
-            fileExists = hasFileDownloadIfNeeded(fileReferenceDownload);
-            if (fileExists) startFileServing(fileReference, receiver, acceptedCompressionTypes);
-        } catch (IllegalArgumentException e) {
-            fileExists = false;
+            boolean fileExists = hasFileDownloadIfNeeded(fileReferenceDownload);
+            if ( ! fileExists) return NOT_FOUND;
+
+            startFileServing(fileReference, receiver, acceptedCompressionTypes);
+        } catch (Exception e) {
             log.warning("Failed serving file reference '" + fileReference + "', request from " + client + " failed with: " + e.getMessage());
+            return TRANSFER_FAILED;
         }
 
-        return (fileExists ? FileApiErrorCodes.OK : FileApiErrorCodes.NOT_FOUND);
+        return OK;
     }
 
     /* Choose the first compression type (list is in preferred order) that matches an accepted compression type, or fail */
diff --git a/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/RpcServer.java b/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/RpcServer.java
index eee7d6ec63d..d26a22284c0 100644
--- a/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/RpcServer.java
+++ b/configserver/src/main/java/com/yahoo/vespa/config/server/rpc/RpcServer.java
@@ -518,7 +518,7 @@ public class RpcServer implements Runnable, ConfigActivationListener, TenantList
             request.parameters().add(new StringValue(fileData.filename()));
             request.parameters().add(new StringValue(fileData.type().name()));
             request.parameters().add(new Int64Value(fileData.size()));
-            // Only add paramter if not gzip, this is default and old clients will not handle the extra parameter
+            // Only add parameter if not gzip, this is default and old clients will not handle the extra parameter
             if (fileData.compressionType() != CompressionType.gzip)
                 request.parameters().add(new StringValue(fileData.compressionType().name()));
             return request;
@@ -532,7 +532,7 @@ public class RpcServer implements Runnable, ConfigActivationListener, TenantList
             request.parameters().add(new DataValue(buf));
             invokeRpcIfValidConnection(request);
             if (request.isError()) {
-                throw new IllegalArgumentException("Failed delivering reference '" + ref.value() + "' to " +
+                throw new IllegalArgumentException("Failed delivering part of reference '" + ref.value() + "' to " +
                                                            target.toString() + " with error: '" + request.errorMessage() + "'.");
             } else {
                 if (request.returnValues().get(0).asInt32() != 0) {
@@ -550,7 +550,8 @@ public class RpcServer implements Runnable, ConfigActivationListener, TenantList
             request.parameters().add(new StringValue(status.getDescription()));
             invokeRpcIfValidConnection(request);
             if (request.isError()) {
-                throw new IllegalArgumentException("Failed delivering reference '" + fileData.fileReference().value() + "' with file '" + fileData.filename() + "' to " +
+                throw new IllegalArgumentException("Failed delivering eof for reference '" + fileData.fileReference().value() +
+                                                           "' with file '" + fileData.filename() + "' to " +
                                                            target.toString() + " with error: '" + request.errorMessage() + "'.");
             } else {
                 if (request.returnValues().get(0).asInt32() != 0) {
diff --git a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/EmptyFileReferenceData.java b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/EmptyFileReferenceData.java
deleted file mode 100644
index ea8461b42f3..00000000000
--- a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/EmptyFileReferenceData.java
+++ /dev/null
@@ -1,55 +0,0 @@
-// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-package com.yahoo.vespa.filedistribution;
-
-import com.yahoo.config.FileReference;
-
-import java.nio.ByteBuffer;
-
-public class EmptyFileReferenceData extends FileReferenceData {
-
-    private final byte[] content;
-    private final long xxhash;
-    private int contentRead = 0;
-
-    private EmptyFileReferenceData(FileReference fileReference, String filename, Type type, byte[] content, long xxhash) {
-        super(fileReference, filename, type, CompressionType.gzip);
-        this.content = content;
-        this.xxhash = xxhash;
-    }
-
-    public static FileReferenceData empty(FileReference fileReference, String filename) {
-        return new EmptyFileReferenceData(fileReference, filename, FileReferenceData.Type.file, new byte[0], 0);
-    }
-
-    public ByteBuffer content() {
-        return ByteBuffer.wrap(content);
-    }
-
-    @Override
-    public int nextContent(ByteBuffer bb) {
-        if (contentRead >= content.length) {
-            return -1;
-        } else {
-            int left = content.length - contentRead;
-            int size = Math.min(bb.remaining(), left);
-            bb.put(content, contentRead, size);
-            contentRead += size;
-            return size;
-        }
-    }
-
-    @Override
-    public long xxhash() {
-        return xxhash;
-    }
-
-    @Override
-    public long size() {
-        return content.length;
-    }
-
-    @Override
-    public void close() {
-        // no-op
-    }
-}
diff --git a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReceiver.java b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReceiver.java
index b37fe02226b..a567a3bc4b3 100644
--- a/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReceiver.java
+++ b/filedistribution/src/main/java/com/yahoo/vespa/filedistribution/FileReceiver.java
@@ -243,7 +243,7 @@ public class FileReceiver {
         synchronized (sessions) {
             if (sessions.containsKey(sessionId)) {
                 retval = 1;
-                log.severe("Session id " + sessionId + " already exist, impossible. Request from(" + req.target() + ")");
+                log.severe("Session id " + sessionId + " already exist, impossible. Request from " + req.target());
             } else {
                 try {
                     sessions.put(sessionId, new Session(downloadDirectory, sessionId, reference,