[
https://issues.apache.org/jira/browse/SCB-887?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16604208#comment-16604208
]
ASF GitHub Bot commented on SCB-887:
------------------------------------
liubao68 closed pull request #891: [SCB-887]aysnc servlet timeout is too short
and may block container pool when tasks are timeout
URL: https://github.com/apache/incubator-servicecomb-java-chassis/pull/891
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git
a/common/common-rest/src/main/java/org/apache/servicecomb/common/rest/AbstractRestInvocation.java
b/common/common-rest/src/main/java/org/apache/servicecomb/common/rest/AbstractRestInvocation.java
index a467f0bf1..79d039355 100644
---
a/common/common-rest/src/main/java/org/apache/servicecomb/common/rest/AbstractRestInvocation.java
+++
b/common/common-rest/src/main/java/org/apache/servicecomb/common/rest/AbstractRestInvocation.java
@@ -124,6 +124,9 @@ protected void scheduleInvocation() {
operationMeta.getExecutor().execute(() -> {
synchronized (this.requestEx) {
try {
+ if (isInQueueTimeout()) {
+ throw new InvocationException(Status.INTERNAL_SERVER_ERROR,
"Timeout when processing the request.");
+ }
if (requestEx.getAttribute(RestConst.REST_REQUEST) != requestEx) {
// already timeout
// in this time, request maybe recycled and reused by web
container, do not use requestEx
@@ -142,6 +145,11 @@ protected void scheduleInvocation() {
});
}
+ private boolean isInQueueTimeout() {
+ return System.nanoTime() - invocation.getStartTime() >
+ CommonRestConfig.getRequestWaitInPoolTimeout() * 1_000_000;
+ }
+
protected void runOnExecutor() {
invocation.onStartExecute();
diff --git
a/common/common-rest/src/main/java/org/apache/servicecomb/common/rest/CommonRestConfig.java
b/common/common-rest/src/main/java/org/apache/servicecomb/common/rest/CommonRestConfig.java
new file mode 100644
index 000000000..a673a5acc
--- /dev/null
+++
b/common/common-rest/src/main/java/org/apache/servicecomb/common/rest/CommonRestConfig.java
@@ -0,0 +1,35 @@
+/*
+ * 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.
+ */
+
+package org.apache.servicecomb.common.rest;
+
+import com.netflix.config.DynamicLongProperty;
+import com.netflix.config.DynamicPropertyFactory;
+
+public class CommonRestConfig {
+ public static final String KEY_SERVICECOMB_REQUEST_WAIT_IN_POOL_TIMEOUT =
"servicecomb.rest.server.requestWaitInPoolTimeout";
+
+ public static final long DEFAULT_REQUEST_WAIT_IN_POOL_TIMEOUT = 30000;
+
+ private static final DynamicLongProperty requestWaitInPoolTimeoutProperty =
+
DynamicPropertyFactory.getInstance().getLongProperty(KEY_SERVICECOMB_REQUEST_WAIT_IN_POOL_TIMEOUT,
+ DEFAULT_REQUEST_WAIT_IN_POOL_TIMEOUT);
+
+ public static long getRequestWaitInPoolTimeout() {
+ return requestWaitInPoolTimeoutProperty.get();
+ }
+}
diff --git
a/common/common-rest/src/main/java/org/apache/servicecomb/common/rest/RestProducerInvocation.java
b/common/common-rest/src/main/java/org/apache/servicecomb/common/rest/RestProducerInvocation.java
index f3cce625e..965cebf13 100644
---
a/common/common-rest/src/main/java/org/apache/servicecomb/common/rest/RestProducerInvocation.java
+++
b/common/common-rest/src/main/java/org/apache/servicecomb/common/rest/RestProducerInvocation.java
@@ -31,6 +31,7 @@
import org.apache.servicecomb.swagger.invocation.exception.InvocationException;
public class RestProducerInvocation extends AbstractRestInvocation {
+
protected Transport transport;
public void invoke(Transport transport, HttpServletRequestEx requestEx,
HttpServletResponseEx responseEx,
diff --git
a/common/common-rest/src/test/java/org/apache/servicecomb/common/rest/TestCommonRestConfig.java
b/common/common-rest/src/test/java/org/apache/servicecomb/common/rest/TestCommonRestConfig.java
new file mode 100644
index 000000000..d3bb2a276
--- /dev/null
+++
b/common/common-rest/src/test/java/org/apache/servicecomb/common/rest/TestCommonRestConfig.java
@@ -0,0 +1,37 @@
+/*
+ * 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.
+ */
+
+package org.apache.servicecomb.common.rest;
+
+import org.apache.servicecomb.foundation.test.scaffolding.config.ArchaiusUtils;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class TestCommonRestConfig {
+ @After
+ public void tearDown() {
+ ArchaiusUtils.resetConfig();
+ }
+
+ @Test
+ public void testTimeoutConfig() {
+ Assert.assertEquals(CommonRestConfig.getRequestWaitInPoolTimeout(), 30000);
+
ArchaiusUtils.setProperty(CommonRestConfig.KEY_SERVICECOMB_REQUEST_WAIT_IN_POOL_TIMEOUT,
50000);
+ Assert.assertEquals(CommonRestConfig.getRequestWaitInPoolTimeout(), 50000);
+ }
+}
diff --git a/transports/transport-highway/pom.xml
b/transports/transport-highway/pom.xml
index e33858242..2824517b9 100644
--- a/transports/transport-highway/pom.xml
+++ b/transports/transport-highway/pom.xml
@@ -52,5 +52,9 @@
<groupId>org.apache.servicecomb</groupId>
<artifactId>foundation-metrics</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.servicecomb</groupId>
+ <artifactId>foundation-test-scaffolding</artifactId>
+ </dependency>
</dependencies>
</project>
diff --git
a/transports/transport-highway/src/main/java/org/apache/servicecomb/transport/highway/HighwayConfig.java
b/transports/transport-highway/src/main/java/org/apache/servicecomb/transport/highway/HighwayConfig.java
index b0583902d..35ea257b4 100644
---
a/transports/transport-highway/src/main/java/org/apache/servicecomb/transport/highway/HighwayConfig.java
+++
b/transports/transport-highway/src/main/java/org/apache/servicecomb/transport/highway/HighwayConfig.java
@@ -18,6 +18,7 @@
package org.apache.servicecomb.transport.highway;
import com.netflix.config.DynamicIntProperty;
+import com.netflix.config.DynamicLongProperty;
import com.netflix.config.DynamicPropertyFactory;
import com.netflix.config.DynamicStringProperty;
@@ -25,6 +26,18 @@
private HighwayConfig() {
}
+ public static final String KEY_SERVICECOMB_REQUEST_WAIT_IN_POOL_TIMEOUT =
"servicecomb.highway.server.requestWaitInPoolTimeout";
+
+ public static final long DEFAULT_REQUEST_WAIT_IN_POOL_TIMEOUT = 30000;
+
+ private static final DynamicLongProperty requestWaitInPoolTimeoutProperty =
+
DynamicPropertyFactory.getInstance().getLongProperty(KEY_SERVICECOMB_REQUEST_WAIT_IN_POOL_TIMEOUT,
+ DEFAULT_REQUEST_WAIT_IN_POOL_TIMEOUT);
+
+ public static long getRequestWaitInPoolTimeout() {
+ return requestWaitInPoolTimeoutProperty.get();
+ }
+
public static String getAddress() {
DynamicStringProperty address =
DynamicPropertyFactory.getInstance().getStringProperty("servicecomb.highway.address",
null);
diff --git
a/transports/transport-highway/src/main/java/org/apache/servicecomb/transport/highway/HighwayServerInvoke.java
b/transports/transport-highway/src/main/java/org/apache/servicecomb/transport/highway/HighwayServerInvoke.java
index 4878c415d..eee779885 100644
---
a/transports/transport-highway/src/main/java/org/apache/servicecomb/transport/highway/HighwayServerInvoke.java
+++
b/transports/transport-highway/src/main/java/org/apache/servicecomb/transport/highway/HighwayServerInvoke.java
@@ -19,6 +19,8 @@
import java.util.Map;
+import javax.ws.rs.core.Response.Status;
+
import org.apache.servicecomb.codec.protobuf.definition.OperationProtobuf;
import org.apache.servicecomb.codec.protobuf.definition.ProtobufManager;
import org.apache.servicecomb.codec.protobuf.utils.WrapSchema;
@@ -106,6 +108,9 @@ private void doInit(TcpConnection connection, long msgId,
RequestHeader header,
private void runInExecutor() {
try {
+ if (isInQueueTimeout()) {
+ throw new InvocationException(Status.INTERNAL_SERVER_ERROR, "Timeout
when processing the request.");
+ }
doRunInExecutor();
} catch (Throwable e) {
String msg = String.format("handle request error, %s, msgId=%d",
@@ -117,6 +122,11 @@ private void runInExecutor() {
}
}
+ private boolean isInQueueTimeout() {
+ return System.nanoTime() - invocation.getStartTime() >
+ HighwayConfig.getRequestWaitInPoolTimeout() * 1_000_000;
+ }
+
private void doRunInExecutor() throws Exception {
invocation.onStartExecute();
diff --git
a/transports/transport-highway/src/test/java/org/apache/servicecomb/transport/highway/TestHighwayConfig.java
b/transports/transport-highway/src/test/java/org/apache/servicecomb/transport/highway/TestHighwayConfig.java
index 8c490da20..31889e5bc 100644
---
a/transports/transport-highway/src/test/java/org/apache/servicecomb/transport/highway/TestHighwayConfig.java
+++
b/transports/transport-highway/src/test/java/org/apache/servicecomb/transport/highway/TestHighwayConfig.java
@@ -17,10 +17,16 @@
package org.apache.servicecomb.transport.highway;
+import org.apache.servicecomb.foundation.test.scaffolding.config.ArchaiusUtils;
+import org.junit.After;
import org.junit.Assert;
import org.junit.Test;
public class TestHighwayConfig {
+ @After
+ public void tearDown() {
+ ArchaiusUtils.resetConfig();
+ }
@Test
public void getServerThreadCount() {
Assert.assertEquals(HighwayConfig.getServerThreadCount(), 1);
@@ -35,4 +41,11 @@ public void getClientThreadCount() {
public void getAddress() {
Assert.assertEquals(HighwayConfig.getAddress(), null);
}
+
+ @Test
+ public void testTimeoutConfig() {
+ Assert.assertEquals(HighwayConfig.getRequestWaitInPoolTimeout(), 30000);
+
ArchaiusUtils.setProperty(HighwayConfig.KEY_SERVICECOMB_REQUEST_WAIT_IN_POOL_TIMEOUT,
50000);
+ Assert.assertEquals(HighwayConfig.getRequestWaitInPoolTimeout(), 50000);
+ }
}
diff --git
a/transports/transport-rest/transport-rest-servlet/src/main/java/org/apache/servicecomb/transport/rest/servlet/RestAsyncListener.java
b/transports/transport-rest/transport-rest-servlet/src/main/java/org/apache/servicecomb/transport/rest/servlet/RestAsyncListener.java
index b2bcd7f46..3e2c9dbf5 100644
---
a/transports/transport-rest/transport-rest-servlet/src/main/java/org/apache/servicecomb/transport/rest/servlet/RestAsyncListener.java
+++
b/transports/transport-rest/transport-rest-servlet/src/main/java/org/apache/servicecomb/transport/rest/servlet/RestAsyncListener.java
@@ -26,6 +26,7 @@
import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletResponse;
import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.Response.Status;
import org.apache.servicecomb.common.rest.RestConst;
import org.apache.servicecomb.foundation.common.utils.JsonUtils;
@@ -44,7 +45,7 @@
static {
try {
- TIMEOUT_MESSAGE = JsonUtils.writeValueAsString(new
CommonExceptionData("TimeOut in Processing"));
+ TIMEOUT_MESSAGE = JsonUtils.writeValueAsString(new
CommonExceptionData("Timeout when processing the request."));
} catch (JsonProcessingException e) {
LOGGER.error("Failed to init timeout message.", e);
}
@@ -63,17 +64,19 @@ public void onTimeout(AsyncEvent event) throws IOException {
// to avoid concurrent, must lock request
ServletRequest request = event.getSuppliedRequest();
HttpServletRequestEx requestEx = (HttpServletRequestEx)
request.getAttribute(RestConst.REST_REQUEST);
+ LOGGER.error("Rest request timeout, method {}, path {}.",
requestEx.getMethod(), requestEx.getRequestURI());
+ // Waiting till executing in executor done. This operation may block
container pool and make timeout requests in executor's
+ // queue getting executed, and will cause bad performance. So default
timeout is setting to -1 to disable timeout.
synchronized (requestEx) {
ServletResponse response = event.getAsyncContext().getResponse();
if (!response.isCommitted()) {
- LOGGER.error("Rest request timeout, method {}, path {}.",
requestEx.getMethod(), requestEx.getRequestURI());
// invocation in executor's queue
response.setContentType(MediaType.APPLICATION_JSON);
// we don't know if developers declared one statusCode in contract
// so we use cse inner statusCode here
- ((HttpServletResponse)
response).setStatus(ExceptionFactory.PRODUCER_INNER_STATUS_CODE);
+ ((HttpServletResponse)
response).setStatus(Status.INTERNAL_SERVER_ERROR.getStatusCode());
PrintWriter out = response.getWriter();
out.write(TIMEOUT_MESSAGE);
response.flushBuffer();
@@ -81,6 +84,8 @@ public void onTimeout(AsyncEvent event) throws IOException {
request.removeAttribute(RestConst.REST_REQUEST);
}
+
+ LOGGER.error("Rest request timeout committed, method {}, path {}.",
requestEx.getMethod(), requestEx.getRequestURI());
}
@Override
diff --git
a/transports/transport-rest/transport-rest-servlet/src/main/java/org/apache/servicecomb/transport/rest/servlet/ServletConfig.java
b/transports/transport-rest/transport-rest-servlet/src/main/java/org/apache/servicecomb/transport/rest/servlet/ServletConfig.java
index 6f658e704..9993a3dfb 100644
---
a/transports/transport-rest/transport-rest-servlet/src/main/java/org/apache/servicecomb/transport/rest/servlet/ServletConfig.java
+++
b/transports/transport-rest/transport-rest-servlet/src/main/java/org/apache/servicecomb/transport/rest/servlet/ServletConfig.java
@@ -22,23 +22,25 @@
import com.netflix.config.DynamicStringProperty;
public final class ServletConfig {
- static final long DEFAULT_TIMEOUT = 3000;
+ public static final long DEFAULT_ASYN_SERVLET_TIMEOUT = -1;
public static final String KEY_SERVLET_URL_PATTERN =
"servicecomb.rest.servlet.urlPattern";
public static final String SERVICECOMB_REST_ADDRESS =
"servicecomb.rest.address";
- public static final String KEY_SERVICECOMB_REST_SERVER_TIMEOUT =
"servicecomb.rest.server.timeout";
+ public static final String KEY_SERVICECOMB_ASYC_SERVLET_TIMEOUT =
"servicecomb.rest.server.timeout";
public static final String DEFAULT_URL_PATTERN = "/*";
+ private static final DynamicLongProperty asyncServletTimeoutProperty =
+
DynamicPropertyFactory.getInstance().getLongProperty(KEY_SERVICECOMB_ASYC_SERVLET_TIMEOUT,
+ DEFAULT_ASYN_SERVLET_TIMEOUT);
+
private ServletConfig() {
}
- public static long getServerTimeout() {
- DynamicLongProperty address =
-
DynamicPropertyFactory.getInstance().getLongProperty(KEY_SERVICECOMB_REST_SERVER_TIMEOUT,
DEFAULT_TIMEOUT);
- return address.get();
+ public static long getAsyncServletTimeout() {
+ return asyncServletTimeoutProperty.get();
}
public static String getLocalServerAddress() {
diff --git
a/transports/transport-rest/transport-rest-servlet/src/main/java/org/apache/servicecomb/transport/rest/servlet/ServletRestDispatcher.java
b/transports/transport-rest/transport-rest-servlet/src/main/java/org/apache/servicecomb/transport/rest/servlet/ServletRestDispatcher.java
index 3ec2a7640..01dd4b17f 100644
---
a/transports/transport-rest/transport-rest-servlet/src/main/java/org/apache/servicecomb/transport/rest/servlet/ServletRestDispatcher.java
+++
b/transports/transport-rest/transport-rest-servlet/src/main/java/org/apache/servicecomb/transport/rest/servlet/ServletRestDispatcher.java
@@ -48,7 +48,7 @@ public void service(HttpServletRequest request,
HttpServletResponse response) {
// 异步场景
final AsyncContext asyncCtx = request.startAsync();
asyncCtx.addListener(restAsyncListener);
- asyncCtx.setTimeout(ServletConfig.getServerTimeout());
+ asyncCtx.setTimeout(ServletConfig.getAsyncServletTimeout());
HttpServletRequestEx requestEx = new StandardHttpServletRequestEx(request);
HttpServletResponseEx responseEx = new
StandardHttpServletResponseEx(response);
diff --git
a/transports/transport-rest/transport-rest-servlet/src/test/java/org/apache/servicecomb/transport/rest/servlet/TestRestAsyncListener.java
b/transports/transport-rest/transport-rest-servlet/src/test/java/org/apache/servicecomb/transport/rest/servlet/TestRestAsyncListener.java
index 3af864c76..af533abf3 100644
---
a/transports/transport-rest/transport-rest-servlet/src/test/java/org/apache/servicecomb/transport/rest/servlet/TestRestAsyncListener.java
+++
b/transports/transport-rest/transport-rest-servlet/src/test/java/org/apache/servicecomb/transport/rest/servlet/TestRestAsyncListener.java
@@ -126,8 +126,8 @@ public void onTimeoutNotCommitted() throws IOException {
Assert.assertNull(request.getAttribute(RestConst.REST_REQUEST));
Assert.assertEquals(MediaType.APPLICATION_JSON, contentType);
- Assert.assertEquals(ExceptionFactory.PRODUCER_INNER_STATUS_CODE,
statusCode);
+ Assert.assertEquals(500, statusCode);
Assert.assertTrue(flushed);
- Assert.assertEquals("{\"message\":\"TimeOut in Processing\"}",
writer.toString());
+ Assert.assertEquals("{\"message\":\"Timeout when processing the
request.\"}", writer.toString());
}
}
diff --git
a/transports/transport-rest/transport-rest-servlet/src/test/java/org/apache/servicecomb/transport/rest/servlet/TestServletConfig.java
b/transports/transport-rest/transport-rest-servlet/src/test/java/org/apache/servicecomb/transport/rest/servlet/TestServletConfig.java
index fec0968e1..4b0bda906 100644
---
a/transports/transport-rest/transport-rest-servlet/src/test/java/org/apache/servicecomb/transport/rest/servlet/TestServletConfig.java
+++
b/transports/transport-rest/transport-rest-servlet/src/test/java/org/apache/servicecomb/transport/rest/servlet/TestServletConfig.java
@@ -44,7 +44,7 @@ public void testGetLocalServerAddress() {
@Test
public void testGetServerTimeout() {
- Assert.assertEquals(ServletConfig.DEFAULT_TIMEOUT,
ServletConfig.getServerTimeout());
+ Assert.assertEquals(ServletConfig.DEFAULT_ASYN_SERVLET_TIMEOUT,
ServletConfig.getAsyncServletTimeout());
}
@Test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
> aysnc servlet timeout is too short and may block container pool when tasks
> are timeout
> --------------------------------------------------------------------------------------
>
> Key: SCB-887
> URL: https://issues.apache.org/jira/browse/SCB-887
> Project: Apache ServiceComb
> Issue Type: Improvement
> Components: Java-Chassis
> Affects Versions: java-chassis-1.0.0
> Reporter: liubao
> Assignee: liubao
> Priority: Major
>
> # aysnc servlet timeout is too short, and this is not applicable for time
> consuming tasks.
> # If setting timeout too long, may block container pool and time out
> requests in queue get executed
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
