This is an automated email from the ASF dual-hosted git repository.
liubao pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/servicecomb-java-chassis.git
The following commit(s) were added to refs/heads/master by this push:
new 1acfe7d [SCB-2186] fix bug of ExceptionConverter throw exception
(#2219)
1acfe7d is described below
commit 1acfe7d06d420795c47b3e6ab6013242cabe7a9d
Author: wujimin <[email protected]>
AuthorDate: Thu Jan 28 09:07:40 2021 +0800
[SCB-2186] fix bug of ExceptionConverter throw exception (#2219)
---
.../servicecomb/core/exception/Exceptions.java | 20 +++++++++--
.../servicecomb/core/exception/ExceptionsTest.java | 40 ++++++++++++++++++++++
.../foundation/common/utils/ReflectUtils.java | 11 ------
.../metrics/core/OsMetersInitializer.java | 11 +++++-
.../metrics/core/TestOsMeterInitializer.java | 20 ++---------
...g.apache.servicecomb.core.filter.FilterProvider | 18 ----------
6 files changed, 70 insertions(+), 50 deletions(-)
diff --git
a/core/src/main/java/org/apache/servicecomb/core/exception/Exceptions.java
b/core/src/main/java/org/apache/servicecomb/core/exception/Exceptions.java
index 7166d6b..c3f6059 100644
--- a/core/src/main/java/org/apache/servicecomb/core/exception/Exceptions.java
+++ b/core/src/main/java/org/apache/servicecomb/core/exception/Exceptions.java
@@ -18,6 +18,7 @@ package org.apache.servicecomb.core.exception;
import static javax.ws.rs.core.Response.Status.BAD_REQUEST;
import static javax.ws.rs.core.Response.Status.INTERNAL_SERVER_ERROR;
+import static org.apache.commons.lang3.exception.ExceptionUtils.getStackTrace;
import static
org.apache.servicecomb.core.exception.ExceptionCodes.GENERIC_CLIENT;
import static
org.apache.servicecomb.core.exception.ExceptionCodes.GENERIC_SERVER;
import static
org.apache.servicecomb.swagger.invocation.InvocationType.CONSUMER;
@@ -35,6 +36,7 @@ import
org.apache.servicecomb.foundation.common.concurrent.ConcurrentHashMapEx;
import org.apache.servicecomb.foundation.common.utils.ExceptionUtils;
import org.apache.servicecomb.foundation.common.utils.SPIServiceUtils;
import org.apache.servicecomb.swagger.invocation.Response;
+import org.apache.servicecomb.swagger.invocation.exception.CommonExceptionData;
import org.apache.servicecomb.swagger.invocation.exception.ExceptionFactory;
import org.apache.servicecomb.swagger.invocation.exception.InvocationException;
import org.slf4j.Logger;
@@ -141,9 +143,23 @@ public final class Exceptions {
public static InvocationException convert(@Nullable Invocation invocation,
Throwable throwable,
StatusType genericStatus) {
+ return convert(invocation, throwable, genericStatus, CACHE);
+ }
+
+ public static InvocationException convert(@Nullable Invocation invocation,
Throwable throwable,
+ StatusType genericStatus, Map<Class<?>, ExceptionConverter<Throwable>>
cache) {
Throwable unwrapped = unwrap(throwable);
- return CACHE.computeIfAbsent(unwrapped.getClass(), clazz ->
findConverter(unwrapped))
- .convert(invocation, unwrapped, genericStatus);
+ try {
+ return cache.computeIfAbsent(unwrapped.getClass(), clazz ->
findConverter(unwrapped))
+ .convert(invocation, unwrapped, genericStatus);
+ } catch (Exception e) {
+ LOGGER.error("BUG: ExceptionConverter.convert MUST not throw exception,
please fix it.\n"
+ + "original exception:{}converter exception:{}",
+ getStackTrace(throwable),
+ getStackTrace(e));
+ return new InvocationException(INTERNAL_SERVER_ERROR,
+ new CommonExceptionData(GENERIC_SERVER,
INTERNAL_SERVER_ERROR.getReasonPhrase()));
+ }
}
private static ExceptionConverter<Throwable> findConverter(Throwable
throwable) {
diff --git
a/core/src/test/java/org/apache/servicecomb/core/exception/ExceptionsTest.java
b/core/src/test/java/org/apache/servicecomb/core/exception/ExceptionsTest.java
index fa6783f..9350ae1 100644
---
a/core/src/test/java/org/apache/servicecomb/core/exception/ExceptionsTest.java
+++
b/core/src/test/java/org/apache/servicecomb/core/exception/ExceptionsTest.java
@@ -21,6 +21,15 @@ import static javax.ws.rs.core.Response.Status.BAD_REQUEST;
import static javax.ws.rs.core.Response.Status.INTERNAL_SERVER_ERROR;
import static org.assertj.core.api.Assertions.assertThat;
+import java.util.HashMap;
+import java.util.Map;
+
+import javax.annotation.Nullable;
+import javax.ws.rs.core.Response.StatusType;
+
+import org.apache.servicecomb.core.Invocation;
+import
org.apache.servicecomb.foundation.test.scaffolding.exception.RuntimeExceptionWithoutStackTrace;
+import org.apache.servicecomb.foundation.test.scaffolding.log.LogCollector;
import org.apache.servicecomb.swagger.invocation.exception.CommonExceptionData;
import org.apache.servicecomb.swagger.invocation.exception.InvocationException;
import org.junit.jupiter.api.Test;
@@ -61,4 +70,35 @@ class ExceptionsTest {
assertThat(Json.encode(invocationException.getErrorData()))
.isEqualTo("{\"code\":\"SCB.50000000\",\"message\":\"msg\"}");
}
+
+ static class ThrowExceptionWhenConvert implements
ExceptionConverter<Throwable> {
+ @Override
+ public boolean canConvert(Throwable throwable) {
+ return true;
+ }
+
+ @Override
+ public InvocationException convert(@Nullable Invocation invocation,
Throwable throwable, StatusType genericStatus) {
+ throw new RuntimeExceptionWithoutStackTrace("mock exception when
convert");
+ }
+ }
+
+ @Test
+ void should_protect_when_converter_throw_exception() {
+ try (LogCollector logCollector = new LogCollector()) {
+ Map<Class<?>, ExceptionConverter<Throwable>> cache = new HashMap<>();
+ cache.put(RuntimeExceptionWithoutStackTrace.class, new
ThrowExceptionWhenConvert());
+ InvocationException exception = Exceptions
+ .convert(null, new RuntimeExceptionWithoutStackTrace("exception need
convert"), BAD_REQUEST, cache);
+
+ assertThat(exception.getStatus())
+ .isSameAs(INTERNAL_SERVER_ERROR);
+ assertThat(exception.getErrorData().toString())
+ .isEqualTo("CommonExceptionData{code='SCB.50000000',
message='Internal Server Error', dynamic={}}");
+
assertThat(logCollector.getLastEvents().getRenderedMessage().replace("\r\n",
"\n"))
+ .isEqualTo("BUG: ExceptionConverter.convert MUST not throw
exception, please fix it.\n"
+ + "original
exception:org.apache.servicecomb.foundation.test.scaffolding.exception.RuntimeExceptionWithoutStackTrace:
exception need convert\n"
+ + "converter
exception:org.apache.servicecomb.foundation.test.scaffolding.exception.RuntimeExceptionWithoutStackTrace:
mock exception when convert\n");
+ }
+ }
}
\ No newline at end of file
diff --git
a/foundations/foundation-common/src/main/java/org/apache/servicecomb/foundation/common/utils/ReflectUtils.java
b/foundations/foundation-common/src/main/java/org/apache/servicecomb/foundation/common/utils/ReflectUtils.java
index 9730eda..1b4d3f1 100644
---
a/foundations/foundation-common/src/main/java/org/apache/servicecomb/foundation/common/utils/ReflectUtils.java
+++
b/foundations/foundation-common/src/main/java/org/apache/servicecomb/foundation/common/utils/ReflectUtils.java
@@ -21,7 +21,6 @@ import java.lang.reflect.Array;
import java.lang.reflect.Field;
import java.lang.reflect.GenericArrayType;
import java.lang.reflect.Method;
-import java.lang.reflect.Modifier;
import java.lang.reflect.ParameterizedType;
import java.lang.reflect.Type;
@@ -31,13 +30,6 @@ import org.springframework.util.ReflectionUtils;
import com.google.common.reflect.TypeToken;
public final class ReflectUtils {
- private static final Field MODIFIERS_FIELD =
- ReflectionUtils.findField(Field.class, "modifiers");
-
- static {
- MODIFIERS_FIELD.setAccessible(true);
- }
-
private ReflectUtils() {
}
@@ -49,9 +41,6 @@ public final class ReflectUtils {
public static void setField(Class<?> cls, Object instance, String fieldName,
Object value) {
Field field = ReflectionUtils.findField(cls, fieldName);
try {
- if ((field.getModifiers() & Modifier.FINAL) != 0) {
- MODIFIERS_FIELD.setInt(field, field.getModifiers() & ~Modifier.FINAL);
- }
field.setAccessible(true);
field.set(instance, value);
} catch (Exception e) {
diff --git
a/metrics/metrics-core/src/main/java/org/apache/servicecomb/metrics/core/OsMetersInitializer.java
b/metrics/metrics-core/src/main/java/org/apache/servicecomb/metrics/core/OsMetersInitializer.java
index 783d0cc..88f0139 100644
---
a/metrics/metrics-core/src/main/java/org/apache/servicecomb/metrics/core/OsMetersInitializer.java
+++
b/metrics/metrics-core/src/main/java/org/apache/servicecomb/metrics/core/OsMetersInitializer.java
@@ -24,6 +24,7 @@ import org.apache.servicecomb.metrics.core.meter.os.OsMeter;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.eventbus.EventBus;
import com.netflix.spectator.api.Registry;
import com.netflix.spectator.api.SpectatorUtils;
@@ -33,9 +34,17 @@ public class OsMetersInitializer implements
MetricsInitializer {
private OsMeter osMeter;
+ private boolean isOsLinux = SystemUtils.IS_OS_LINUX;
+
+ @VisibleForTesting
+ OsMetersInitializer setOsLinux(boolean osLinux) {
+ isOsLinux = osLinux;
+ return this;
+ }
+
@Override
public void init(GlobalRegistry globalRegistry, EventBus eventBus,
MetricsBootstrapConfig config) {
- if (!SystemUtils.IS_OS_LINUX) {
+ if (!isOsLinux) {
LOGGER.info("only support linux os to collect cpu and net info");
return;
}
diff --git
a/metrics/metrics-core/src/test/java/org/apache/servicecomb/metrics/core/TestOsMeterInitializer.java
b/metrics/metrics-core/src/test/java/org/apache/servicecomb/metrics/core/TestOsMeterInitializer.java
index 2514934..440176a 100644
---
a/metrics/metrics-core/src/test/java/org/apache/servicecomb/metrics/core/TestOsMeterInitializer.java
+++
b/metrics/metrics-core/src/test/java/org/apache/servicecomb/metrics/core/TestOsMeterInitializer.java
@@ -25,17 +25,13 @@ import java.util.List;
import java.util.Map;
import org.apache.commons.io.FileUtils;
-import org.apache.commons.lang3.SystemUtils;
-import org.apache.servicecomb.foundation.common.utils.ReflectUtils;
import org.apache.servicecomb.foundation.metrics.registry.GlobalRegistry;
import org.apache.servicecomb.metrics.core.meter.os.CpuMeter;
import org.apache.servicecomb.metrics.core.meter.os.NetMeter;
import org.apache.servicecomb.metrics.core.meter.os.OsMeter;
import org.apache.servicecomb.metrics.core.meter.os.cpu.CpuUtils;
import org.apache.servicecomb.metrics.core.meter.os.net.InterfaceUsage;
-import org.junit.After;
import org.junit.Assert;
-import org.junit.Before;
import org.junit.Test;
import com.google.common.eventbus.EventBus;
@@ -54,19 +50,11 @@ public class TestOsMeterInitializer {
Registry registry = new DefaultRegistry(globalRegistry.getClock());
- private boolean isLinux;
-
@Mocked
EventBus eventBus;
- @Before
- public void beforeTest() {
- isLinux = SystemUtils.IS_OS_LINUX;
- }
-
@Test
public void init(@Mocked Runtime runtime, @Mocked RuntimeMXBean mxBean) {
- ReflectUtils.setField(SystemUtils.class, null, "IS_OS_LINUX", true);
List<String> list = new ArrayList<>();
list.add("13 1 1 1 1 1 1 1 1 0 0 1 1 1 1 1 1 1 1 1 1 1 1 1");
list.add("useless");
@@ -111,6 +99,7 @@ public class TestOsMeterInitializer {
};
globalRegistry.add(registry);
OsMetersInitializer osMetersInitializer = new OsMetersInitializer();
+ osMetersInitializer.setOsLinux(true);
osMetersInitializer.init(globalRegistry, eventBus, null);
OsMeter osMeter = osMetersInitializer.getOsMeter();
Assert.assertNotNull(osMeter);
@@ -149,13 +138,8 @@ public class TestOsMeterInitializer {
@Test
public void initFail() {
OsMetersInitializer osMetersInitializer = new OsMetersInitializer();
- ReflectUtils.setField(SystemUtils.class, null, "IS_OS_LINUX", false);
+ osMetersInitializer.setOsLinux(false);
osMetersInitializer.init(null, eventBus, null);
Assert.assertNull(osMetersInitializer.getOsMeter());
}
-
- @After
- public void afterTest() {
- ReflectUtils.setField(SystemUtils.class, null, "IS_OS_LINUX", isLinux);
- }
}
diff --git
a/transports/transport-highway/src/main/resources/META-INF/services/org.apache.servicecomb.core.filter.FilterProvider
b/transports/transport-highway/src/main/resources/META-INF/services/org.apache.servicecomb.core.filter.FilterProvider
deleted file mode 100644
index 950dcb7..0000000
---
a/transports/transport-highway/src/main/resources/META-INF/services/org.apache.servicecomb.core.filter.FilterProvider
+++ /dev/null
@@ -1,18 +0,0 @@
-#
-# Licensed to the Apache Software Foundation (ASF) under one or more
-# contributor license agreements. See the NOTICE file distributed with
-# this work for additional information regarding copyright ownership.
-# The ASF licenses this file to You under the Apache License, Version 2.0
-# (the "License"); you may not use this file except in compliance with
-# the License. You may obtain a copy of the License at
-#
-# http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
-#
-
-org.apache.servicecomb.transport.highway.HighwayFilterProvider
\ No newline at end of file
