Peer disconnects should not result in stack traces. (#818)

Exceptions that serve more as flow control than error reporting should not log stack traces.
PegaSysEng · Feb 9, 2019 · 7ab5f60 · 7ab5f60
1 parent 7e9d6b0
commit 7ab5f60
Show file tree

Hide file tree

Showing 5 changed files with 24 additions and 6 deletions.
diff --git a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/manager/AbstractPeerTask.java b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/manager/AbstractPeerTask.java
@@ -49,7 +49,7 @@ protected void executeTask() {
     try {
       executeTaskWithPeer(peer);
     } catch (final PeerNotConnected e) {
-      result.get().completeExceptionally(new PeerDisconnectedException());
+      result.get().completeExceptionally(new PeerDisconnectedException(peer));
     }
   }
 

diff --git a/...src/main/java/tech/pegasys/pantheon/ethereum/eth/manager/exceptions/EthTaskException.java b/...src/main/java/tech/pegasys/pantheon/ethereum/eth/manager/exceptions/EthTaskException.java
@@ -17,7 +17,11 @@ public class EthTaskException extends RuntimeException {
   private final FailureReason failureReason;
 
   EthTaskException(final FailureReason failureReason) {
-    super("Task failed: " + failureReason.name());
+    this("Task failed: " + failureReason.name(), failureReason);
+  }
+
+  EthTaskException(final String message, final FailureReason failureReason) {
+    super(message);
     this.failureReason = failureReason;
   }
 

diff --git a/...java/tech/pegasys/pantheon/ethereum/eth/manager/exceptions/PeerDisconnectedException.java b/...java/tech/pegasys/pantheon/ethereum/eth/manager/exceptions/PeerDisconnectedException.java
@@ -12,9 +12,15 @@
  */
 package tech.pegasys.pantheon.ethereum.eth.manager.exceptions;
 
+import tech.pegasys.pantheon.ethereum.eth.manager.EthPeer;
+
 public class PeerDisconnectedException extends EthTaskException {
 
-  public PeerDisconnectedException() {
-    super(FailureReason.PEER_DISCONNECTED);
+  public PeerDisconnectedException(final EthPeer peer) {
+    super(
+        "Task failed: "
+            + FailureReason.PEER_DISCONNECTED
+            + (peer == null ? " peer <null>" : " peer " + peer.toString()),
+        FailureReason.PEER_DISCONNECTED);
   }
 }
diff --git a/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/ChainDownloader.java b/ethereum/eth/src/main/java/tech/pegasys/pantheon/ethereum/eth/sync/ChainDownloader.java
@@ -18,6 +18,7 @@
 import tech.pegasys.pantheon.ethereum.core.BlockHeader;
 import tech.pegasys.pantheon.ethereum.eth.manager.EthContext;
 import tech.pegasys.pantheon.ethereum.eth.manager.EthPeer;
+import tech.pegasys.pantheon.ethereum.eth.manager.exceptions.EthTaskException;
 import tech.pegasys.pantheon.ethereum.eth.sync.state.SyncState;
 import tech.pegasys.pantheon.ethereum.eth.sync.state.SyncTarget;
 import tech.pegasys.pantheon.ethereum.eth.sync.tasks.WaitForPeersTask;
@@ -105,6 +106,8 @@ private CompletableFuture<?> executeDownload() {
                       LOG.trace("Download cancelled", t);
                     } else if (rootCause instanceof InvalidBlockException) {
                       LOG.debug("Invalid block downloaded", t);
+                    } else if (rootCause instanceof EthTaskException) {
+                      LOG.debug(rootCause.toString());
                     } else {
                       LOG.error("Error encountered while downloading", t);
                     }
@@ -208,7 +211,12 @@ private CompletableFuture<List<Block>> importBlocks(final List<BlockHeader> chec
             clearSyncTarget();
           } else if (t != null || r.isEmpty()) {
             if (t != null) {
-              LOG.error("Encountered error importing blocks", t);
+              final Throwable rootCause = ExceptionUtils.rootCause(t);
+              if (rootCause instanceof EthTaskException) {
+                LOG.debug(rootCause.toString());
+              } else {
+                LOG.error("Encountered error importing blocks", t);
+              }
             }
             if (checkpointHeaderManager.clearImportedCheckpointHeaders()) {
               chainSegmentTimeouts = 0;

diff --git a/...rc/test/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/FullSyncDownloaderTest.java b/...rc/test/java/tech/pegasys/pantheon/ethereum/eth/sync/fullsync/FullSyncDownloaderTest.java
@@ -550,7 +550,7 @@ public void recoversFromSyncTargetDisconnect() {
     // Downloader should recover and sync to next best peer, but it may stall
     // for 10 seconds first (by design).
     await()
-        .atMost(20, TimeUnit.SECONDS)
+        .atMost(30, TimeUnit.SECONDS)
         .untilAsserted(
             () -> {
               secondBestPeer.respond(secondBestResponder);