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 <[email protected]>
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
[email protected].
