geode-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From upthewatersp...@apache.org
Subject [geode] branch develop updated: GEODE-4746: Handle exceptions and return failures to protobuf clients
Date Wed, 28 Feb 2018 21:03:48 GMT
This is an automated email from the ASF dual-hosted git repository.

upthewaterspout pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode.git


The following commit(s) were added to refs/heads/develop by this push:
     new efc413a  GEODE-4746: Handle exceptions and return failures to protobuf clients
efc413a is described below

commit efc413ad68a73f581e9a84856c6ea379016e8b3d
Author: Dan Smith <upthewaterspout@apache.org>
AuthorDate: Mon Feb 26 11:51:46 2018 -0800

    GEODE-4746: Handle exceptions and return failures to protobuf clients
    
    Catching exceptions that happen before sending a response to the
    protobuf client and sending an error message to the client instead of
    closing the connection.
    
    Cleaning up extraneous catch blocks.
---
 .../internal/protocol/protobuf/v1/Failure.java     | 28 +++++++++++++++++-----
 .../protocol/protobuf/v1/ProtobufOpsProcessor.java | 25 +++++++++----------
 .../AbstractFunctionRequestOperationHandler.java   |  6 +----
 .../v1/operations/PutRequestOperationHandler.java  |  9 ++-----
 .../AuthenticationRequestOperationHandler.java     |  3 ++-
 ...rotobufConnectionAuthorizingStateProcessor.java |  2 +-
 .../state/exception/ConnectionStateException.java  |  3 ++-
 ...dException.java => ExceptionWithErrorCode.java} |  7 ++----
 .../exception/OperationNotAuthorizedException.java | 13 +++++++---
 .../protobuf/v1/AuthenticationIntegrationTest.java |  8 +++++++
 .../LocatorConnectionAuthenticationDUnitTest.java  |  3 +++
 .../PutRequestOperationHandlerJUnitTest.java       | 14 -----------
 12 files changed, 66 insertions(+), 55 deletions(-)

diff --git a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/Failure.java
b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/Failure.java
index f999c89..2c8e6c3 100644
--- a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/Failure.java
+++ b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/Failure.java
@@ -18,6 +18,8 @@ import java.util.function.Function;
 
 import org.apache.geode.annotations.Experimental;
 import org.apache.geode.internal.protocol.protobuf.v1.state.exception.ConnectionStateException;
+import org.apache.geode.internal.protocol.protobuf.v1.state.exception.ExceptionWithErrorCode;
+import org.apache.geode.internal.protocol.protobuf.v1.state.exception.OperationNotAuthorizedException;
 
 @Experimental
 public class Failure<SuccessType> implements Result<SuccessType> {
@@ -33,15 +35,29 @@ public class Failure<SuccessType> implements Result<SuccessType>
{
         .build());
   }
 
-  public static <T> Failure<T> of(ConnectionStateException exception) {
-    return of(exception.getErrorCode(), exception.getMessage());
+  public static <T> Failure<T> of(Throwable exception) {
+    BasicTypes.ErrorCode errorCode = getErrorCode(exception);
+
+    return of(errorCode, getErrorMessage(exception));
+  }
+
+  private static BasicTypes.ErrorCode getErrorCode(Throwable exception) {
+    BasicTypes.ErrorCode errorCode = BasicTypes.ErrorCode.SERVER_ERROR;
+    if (exception instanceof ExceptionWithErrorCode) {
+      errorCode = ((ExceptionWithErrorCode) exception).getErrorCode();
+    }
+    return errorCode;
   }
 
-  public static <T> Failure<T> of(Exception exception) {
-    if (exception instanceof ConnectionStateException) {
-      return of((ConnectionStateException) exception);
+  private static String getErrorMessage(Throwable exception) {
+
+    StringBuilder message = new StringBuilder(exception.toString());
+    while (exception.getCause() != null) {
+      message.append(" Caused by: " + exception.getCause());
+      exception = exception.getCause();
     }
-    return of(BasicTypes.ErrorCode.SERVER_ERROR, exception.toString());
+
+    return message.toString();
   }
 
   @Override
diff --git a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/ProtobufOpsProcessor.java
b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/ProtobufOpsProcessor.java
index 60df3c5..1f2d201 100644
--- a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/ProtobufOpsProcessor.java
+++ b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/ProtobufOpsProcessor.java
@@ -16,6 +16,7 @@ package org.apache.geode.internal.protocol.protobuf.v1;
 
 import org.apache.logging.log4j.Logger;
 
+import org.apache.geode.SystemFailure;
 import org.apache.geode.annotations.Experimental;
 import org.apache.geode.internal.exception.InvalidExecutionContextException;
 import org.apache.geode.internal.logging.LogService;
@@ -54,18 +55,18 @@ public class ProtobufOpsProcessor {
       messageExecutionContext.getConnectionStateProcessor().validateOperation(request,
           serializationService, messageExecutionContext, operationContext);
       result = processOperation(request, messageExecutionContext, requestType, operationContext);
-    } catch (OperationNotAuthorizedException e) {
-      // Don't move to a terminating state for authorization state failures
-      logger.warn(e.getMessage());
-      result = Failure.of(e);
-    } catch (EncodingException | DecodingException e) {
-      logger.warn(e.getMessage());
-      result = Failure.of(e);
-    } catch (ConnectionStateException e) {
-      logger.warn(e.getMessage());
-      messageExecutionContext
-          .setConnectionStateProcessor(new ProtobufConnectionTerminatingStateProcessor());
-      result = Failure.of(e);
+    } catch (VirtualMachineError error) {
+      SystemFailure.initiateFailure(error);
+      throw error;
+    } catch (Throwable t) {
+      logger.warn("Failure for request " + request, t);
+      SystemFailure.checkFailure();
+      result = Failure.of(t);
+
+      if (t instanceof ConnectionStateException) {
+        messageExecutionContext
+            .setConnectionStateProcessor(new ProtobufConnectionTerminatingStateProcessor());
+      }
     }
 
     return ((ClientProtocol.Message.Builder) result.map(operationContext.getToResponse(),
diff --git a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/AbstractFunctionRequestOperationHandler.java
b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/AbstractFunctionRequestOperationHandler.java
index f7baef2..c1a475c 100644
--- a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/AbstractFunctionRequestOperationHandler.java
+++ b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/AbstractFunctionRequestOperationHandler.java
@@ -47,7 +47,7 @@ public abstract class AbstractFunctionRequestOperationHandler<Req, Resp>
   @Override
   public Result<Resp> process(ProtobufSerializationService serializationService, Req
request,
       MessageExecutionContext messageExecutionContext)
-      throws InvalidExecutionContextException, DecodingException {
+      throws InvalidExecutionContextException, DecodingException, EncodingException {
 
     final String functionID = getFunctionID(request);
 
@@ -114,10 +114,6 @@ public abstract class AbstractFunctionRequestOperationHandler<Req,
Resp>
       final String message = "Function execution failed: " + ex.toString();
       logger.info(message, ex);
       return Failure.of(BasicTypes.ErrorCode.SERVER_ERROR, message);
-    } catch (EncodingException ex) {
-      final String message = "Encoding failed: " + ex.toString();
-      logger.info(message, ex);
-      return Failure.of(BasicTypes.ErrorCode.SERVER_ERROR, message);
     }
   }
 
diff --git a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/PutRequestOperationHandler.java
b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/PutRequestOperationHandler.java
index df4c254..23448ed 100644
--- a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/PutRequestOperationHandler.java
+++ b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/PutRequestOperationHandler.java
@@ -57,13 +57,8 @@ public class PutRequestOperationHandler
           "Key and value must both be non-NULL");
     }
 
-    try {
-      region.put(decodedKey, decodedValue);
-      return Success.of(RegionAPI.PutResponse.newBuilder().build());
-    } catch (ClassCastException ex) {
-      logger.error("Received Put request with invalid key type: {}", ex);
-      return Failure.of(BasicTypes.ErrorCode.SERVER_ERROR, ex.toString());
-    }
+    region.put(decodedKey, decodedValue);
+    return Success.of(RegionAPI.PutResponse.newBuilder().build());
   }
 
   public static ResourcePermission determineRequiredPermission(RegionAPI.PutRequest request,
diff --git a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/security/AuthenticationRequestOperationHandler.java
b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/security/AuthenticationRequestOperationHandler.java
index 5d7f44c..d323593 100644
--- a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/security/AuthenticationRequestOperationHandler.java
+++ b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/operations/security/AuthenticationRequestOperationHandler.java
@@ -56,7 +56,8 @@ public class AuthenticationRequestOperationHandler implements
       return Success
           .of(ConnectionAPI.AuthenticationResponse.newBuilder().setAuthenticated(true).build());
     } catch (AuthenticationFailedException e) {
-      logger.warn("Authentication failed", e);
+      messageExecutionContext.getStatistics().incAuthenticationFailures();
+      logger.debug("Authentication failed", e);
       messageExecutionContext
           .setConnectionStateProcessor(new ProtobufConnectionTerminatingStateProcessor());
       return Success
diff --git a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/state/ProtobufConnectionAuthorizingStateProcessor.java
b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/state/ProtobufConnectionAuthorizingStateProcessor.java
index 356ce5b..da2e90e 100644
--- a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/state/ProtobufConnectionAuthorizingStateProcessor.java
+++ b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/state/ProtobufConnectionAuthorizingStateProcessor.java
@@ -48,7 +48,7 @@ public class ProtobufConnectionAuthorizingStateProcessor
           operationContext.getFromRequest().apply(message), serializer));
     } catch (NotAuthorizedException e) {
       messageContext.getStatistics().incAuthorizationViolations();
-      throw new OperationNotAuthorizedException(BasicTypes.ErrorCode.AUTHORIZATION_FAILED,
+      throw new OperationNotAuthorizedException(
           "The user is not authorized to complete this operation");
     } finally {
       threadState.restore();
diff --git a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/state/exception/ConnectionStateException.java
b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/state/exception/ConnectionStateException.java
index 8e543b6..8ddffd2 100644
--- a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/state/exception/ConnectionStateException.java
+++ b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/state/exception/ConnectionStateException.java
@@ -17,7 +17,7 @@ package org.apache.geode.internal.protocol.protobuf.v1.state.exception;
 
 import org.apache.geode.internal.protocol.protobuf.v1.BasicTypes;
 
-public class ConnectionStateException extends Exception {
+public class ConnectionStateException extends Exception implements ExceptionWithErrorCode
{
   private final BasicTypes.ErrorCode errorCode;
 
   public ConnectionStateException(BasicTypes.ErrorCode errorCode, String errorMessage) {
@@ -25,6 +25,7 @@ public class ConnectionStateException extends Exception {
     this.errorCode = errorCode;
   }
 
+  @Override
   public BasicTypes.ErrorCode getErrorCode() {
     return errorCode;
   }
diff --git a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/state/exception/OperationNotAuthorizedException.java
b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/state/exception/ExceptionWithErrorCode.java
similarity index 81%
copy from geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/state/exception/OperationNotAuthorizedException.java
copy to geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/state/exception/ExceptionWithErrorCode.java
index f65f88e..6ab8e0d 100644
--- a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/state/exception/OperationNotAuthorizedException.java
+++ b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/state/exception/ExceptionWithErrorCode.java
@@ -14,11 +14,8 @@
  */
 package org.apache.geode.internal.protocol.protobuf.v1.state.exception;
 
-
 import org.apache.geode.internal.protocol.protobuf.v1.BasicTypes;
 
-public class OperationNotAuthorizedException extends ConnectionStateException {
-  public OperationNotAuthorizedException(BasicTypes.ErrorCode errorCode, String errorMessage)
{
-    super(errorCode, errorMessage);
-  }
+public interface ExceptionWithErrorCode {
+  BasicTypes.ErrorCode getErrorCode();
 }
diff --git a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/state/exception/OperationNotAuthorizedException.java
b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/state/exception/OperationNotAuthorizedException.java
index f65f88e..a08872a 100644
--- a/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/state/exception/OperationNotAuthorizedException.java
+++ b/geode-protobuf/src/main/java/org/apache/geode/internal/protocol/protobuf/v1/state/exception/OperationNotAuthorizedException.java
@@ -15,10 +15,17 @@
 package org.apache.geode.internal.protocol.protobuf.v1.state.exception;
 
 
+import org.apache.geode.GemFireException;
 import org.apache.geode.internal.protocol.protobuf.v1.BasicTypes;
 
-public class OperationNotAuthorizedException extends ConnectionStateException {
-  public OperationNotAuthorizedException(BasicTypes.ErrorCode errorCode, String errorMessage)
{
-    super(errorCode, errorMessage);
+public class OperationNotAuthorizedException extends GemFireException
+    implements ExceptionWithErrorCode {
+  public OperationNotAuthorizedException(String errorMessage) {
+    super(errorMessage);
+  }
+
+  @Override
+  public BasicTypes.ErrorCode getErrorCode() {
+    return BasicTypes.ErrorCode.AUTHORIZATION_FAILED;
   }
 }
diff --git a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/AuthenticationIntegrationTest.java
b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/AuthenticationIntegrationTest.java
index 6bf365f..2700c31 100644
--- a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/AuthenticationIntegrationTest.java
+++ b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/AuthenticationIntegrationTest.java
@@ -14,6 +14,7 @@
  */
 package org.apache.geode.internal.protocol.protobuf.v1;
 
+import static org.apache.geode.internal.protocol.protobuf.statistics.ProtobufClientStatistics.PROTOBUF_CLIENT_STATISTICS;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
@@ -33,12 +34,16 @@ import org.junit.Test;
 import org.junit.contrib.java.lang.system.RestoreSystemProperties;
 import org.junit.experimental.categories.Category;
 
+import org.apache.geode.Statistics;
 import org.apache.geode.cache.Cache;
 import org.apache.geode.cache.CacheFactory;
 import org.apache.geode.cache.server.CacheServer;
 import org.apache.geode.distributed.ConfigurationProperties;
 import org.apache.geode.internal.AvailablePortHelper;
 import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.cache.client.protocol.ClientProtocolServiceLoader;
+import org.apache.geode.internal.protocol.protobuf.statistics.ClientStatistics;
+import org.apache.geode.internal.protocol.protobuf.statistics.ProtobufClientStatistics;
 import org.apache.geode.internal.protocol.protobuf.v1.serializer.ProtobufProtocolSerializer;
 import org.apache.geode.management.internal.security.ResourceConstants;
 import org.apache.geode.security.AuthenticationFailedException;
@@ -248,6 +253,9 @@ public class AuthenticationIntegrationTest {
         parseSimpleAuthenticationResponseFromInput();
     assertFalse(authenticationResponse.getAuthenticated());
 
+    Statistics[] stats = cache.getDistributedSystem()
+        .findStatisticsByType(cache.getDistributedSystem().findType(PROTOBUF_CLIENT_STATISTICS));
+    assertEquals(1, stats[0].getLong("authenticationFailures"));
     verifyConnectionClosed();
   }
 
diff --git a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/acceptance/LocatorConnectionAuthenticationDUnitTest.java
b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/acceptance/LocatorConnectionAuthenticationDUnitTest.java
index 3209e97..4a90590 100644
--- a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/acceptance/LocatorConnectionAuthenticationDUnitTest.java
+++ b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/acceptance/LocatorConnectionAuthenticationDUnitTest.java
@@ -36,10 +36,12 @@ import org.apache.geode.internal.protocol.protobuf.v1.ClientProtocol;
 import org.apache.geode.internal.protocol.protobuf.v1.ConnectionAPI;
 import org.apache.geode.internal.protocol.protobuf.v1.MessageUtil;
 import org.apache.geode.internal.protocol.protobuf.v1.serializer.ProtobufProtocolSerializer;
+import org.apache.geode.internal.protocol.protobuf.v1.state.exception.ConnectionStateException;
 import org.apache.geode.internal.protocol.protobuf.v1.utilities.ProtobufRequestUtilities;
 import org.apache.geode.internal.protocol.protobuf.v1.utilities.ProtobufUtilities;
 import org.apache.geode.security.SimpleTestSecurityManager;
 import org.apache.geode.test.dunit.Host;
+import org.apache.geode.test.dunit.IgnoredException;
 import org.apache.geode.test.dunit.cache.internal.JUnit4CacheTestCase;
 import org.apache.geode.test.dunit.rules.DistributedRestoreSystemProperties;
 import org.apache.geode.test.junit.categories.DistributedTest;
@@ -114,6 +116,7 @@ public class LocatorConnectionAuthenticationDUnitTest extends JUnit4CacheTestCas
    */
   @Test
   public void unauthorizedClientCannotGetServersIfSecurityIsEnabled() throws Throwable {
+    IgnoredException.addIgnoredException(ConnectionStateException.class);
     ClientProtocol.Message getServerRequestMessage = ClientProtocol.Message.newBuilder()
         .setGetServerRequest(ProtobufRequestUtilities.createGetServerRequest()).build();
 
diff --git a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/PutRequestOperationHandlerJUnitTest.java
b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/PutRequestOperationHandlerJUnitTest.java
index 2da76b6..39c2583 100644
--- a/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/PutRequestOperationHandlerJUnitTest.java
+++ b/geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/PutRequestOperationHandlerJUnitTest.java
@@ -103,20 +103,6 @@ public class PutRequestOperationHandlerJUnitTest extends OperationHandlerJUnitTe
     assertEquals(BasicTypes.ErrorCode.SERVER_ERROR, errorMessage.getError().getErrorCode());
   }
 
-  @Test
-  public void test_RegionThrowsClasscastException() throws Exception {
-    when(regionMock.put(any(), any())).thenThrow(ClassCastException.class);
-
-    PutRequestOperationHandler operationHandler = new PutRequestOperationHandler();
-    Result result = operationHandler.process(serializationService, generateTestRequest(),
-        TestExecutionContext.getNoAuthCacheExecutionContext(cacheStub));
-
-    assertTrue(result instanceof Failure);
-    ClientProtocol.ErrorResponse errorMessage =
-        (ClientProtocol.ErrorResponse) result.getErrorMessage();
-    assertEquals(BasicTypes.ErrorCode.SERVER_ERROR, errorMessage.getError().getErrorCode());
-  }
-
   private RegionAPI.PutRequest generateTestRequest() throws EncodingException {
     BasicTypes.EncodedValue testKey = serializationService.encode(TEST_KEY);
     BasicTypes.EncodedValue testValue = serializationService.encode(TEST_VALUE);

-- 
To stop receiving notification emails like this one, please contact
upthewaterspout@apache.org.

Mime
View raw message