[mina-sshd] 03/04: [SSHD-1050] Store only the 1st error that occurs before first authentication

[lg...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

lgoldstein pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mina-sshd.git

commit 4fc3e04743987e807087c555187ae1a5d6a1410c
Author: Lyor Goldstein <lg...@apache.org>
AuthorDate: Thu Aug 6 18:05:37 2020 +0300

    [SSHD-1050] Store only the 1st error that occurs before first
    authentication
---
 .../sshd/client/session/ClientSessionImpl.java     | 57 +++++++++-------------
 1 file changed, 24 insertions(+), 33 deletions(-)

diff --git a/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java b/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
index 9bc68a9..a7c07c9 100644
--- a/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
+++ b/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
@@ -19,18 +19,17 @@
 package org.apache.sshd.client.session;
 
 import java.io.IOException;
-import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.EnumSet;
 import java.util.HashMap;
-import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
 
 import org.apache.sshd.client.ClientFactoryManager;
 import org.apache.sshd.client.future.AuthFuture;
@@ -60,11 +59,8 @@ public class ClientSessionImpl extends AbstractClientSession {
      */
     private volatile AuthFuture authFuture;
 
-    /** Records exceptions before there is an authFuture. */
-    private List<Throwable> earlyErrors = new ArrayList<>();
-
-    /** Guards setting an earlyError and the authFuture together. */
-    private final Object errorLock = new Object();
+    /** Also guards setting an earlyError and the authFuture together. */
+    private final AtomicReference<Throwable> authErrorHolder = new AtomicReference<>();
 
     /**
      * For clients to store their own metadata
@@ -134,33 +130,27 @@ public class ClientSessionImpl extends AbstractClientSession {
 
         ClientUserAuthService authService = getUserAuthService();
         String serviceName = nextServiceName();
-        List<Throwable> errors;
+        Throwable earlyError;
         AuthFuture future;
         // Guard both getting early errors and setting authFuture
-        synchronized (errorLock) {
+        synchronized (authErrorHolder) {
             future = ValidateUtils.checkNotNull(
                     authService.auth(serviceName), "No auth future generated by service=%s", serviceName);
-            errors = earlyErrors;
-            earlyErrors = null;
+            earlyError = authErrorHolder.getAndSet(null);
             authFuture = future;
         }
-        if (errors != null && !errors.isEmpty()) {
-            Iterator<Throwable> iter = errors.iterator();
-            Throwable first = iter.next();
-            iter.forEachRemaining(t -> {
-                if (t != first && t != null) {
-                    first.addSuppressed(t);
-                }
-            });
-            future.setException(first);
+
+        if (earlyError != null) {
+            future.setException(earlyError);
             if (log.isDebugEnabled()) {
-                log.debug("auth({}) early exception type={}: {}", //$NON-NLS-1$
-                        this, first.getClass().getSimpleName(),
-                        first.getMessage());
+                log.debug("auth({}) early exception type={}: {}",
+                        this, earlyError.getClass().getSimpleName(),
+                        earlyError.getMessage());
             }
-            // Or throw directly:
-            // throw new IOException(first.getMessage(), first);
+            // TODO consider throw directly:
+            // throw new IOException(earlyError.getMessage(), earlyError);
         }
+
         return future;
     }
 
@@ -184,22 +174,23 @@ public class ClientSessionImpl extends AbstractClientSession {
 
     protected void signalAuthFailure(Throwable t) {
         AuthFuture future = authFuture;
+        boolean firstError = false;
         if (future == null) {
-            synchronized (errorLock) {
-                if (earlyErrors != null) {
-                    earlyErrors.add(t);
-                }
+            synchronized (authErrorHolder) {
+                // save only the 1st newly signaled exception
+                firstError = authErrorHolder.compareAndSet(null, t);
                 future = authFuture;
             }
         }
+
         if (future != null) {
             future.setException(t);
         }
+
         if (log.isDebugEnabled()) {
-            boolean signalled = future != null && t == future.getException();
-            log.debug("signalAuthFailure({}) type={}, signalled={}: {}", this,
-                    t.getClass().getSimpleName(), Boolean.valueOf(signalled),
-                    t.getMessage());
+            boolean signalled = (future != null) && (t == future.getException());
+            log.debug("signalAuthFailure({}) type={}, signalled={}, first={}: {}",
+                    this, t.getClass().getSimpleName(), signalled, firstError, t.getMessage());
         }
     }