You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hc.apache.org by ol...@apache.org on 2019/10/17 14:38:00 UTC
[httpcomponents-core] 01/02: HTTPCORE-607: HttpAsyncService
incorrectly attempts to submit a response over idle connection in case of a
timeout (no prior request);
cleaned up exception handling logic in HttpAsyncService;
reverted HTTPCORE-482
This is an automated email from the ASF dual-hosted git repository.
olegk pushed a commit to branch HTTPCORE-607
in repository https://gitbox.apache.org/repos/asf/httpcomponents-core.git
commit ef85213d46f87303efb404f3ad9402b27f3dde03
Author: Oleg Kalnichevski <ol...@apache.org>
AuthorDate: Thu Oct 17 16:30:06 2019 +0200
HTTPCORE-607: HttpAsyncService incorrectly attempts to submit a response over idle connection in case of a timeout (no prior request); cleaned up exception handling logic in HttpAsyncService; reverted HTTPCORE-482
---
.../impl/nio/DefaultNHttpServerConnection.java | 1 -
.../apache/http/nio/protocol/HttpAsyncService.java | 91 ++++++++++++----------
.../http/nio/protocol/TestHttpAsyncService.java | 19 ++++-
3 files changed, 66 insertions(+), 45 deletions(-)
diff --git a/httpcore-nio/src/main/java/org/apache/http/impl/nio/DefaultNHttpServerConnection.java b/httpcore-nio/src/main/java/org/apache/http/impl/nio/DefaultNHttpServerConnection.java
index 8d7ee6b..30fe46f 100644
--- a/httpcore-nio/src/main/java/org/apache/http/impl/nio/DefaultNHttpServerConnection.java
+++ b/httpcore-nio/src/main/java/org/apache/http/impl/nio/DefaultNHttpServerConnection.java
@@ -290,7 +290,6 @@ public class DefaultNHttpServerConnection
}
}
} catch (final HttpException ex) {
- resetInput();
handler.exception(this, ex);
} catch (final Exception ex) {
handler.exception(this, ex);
diff --git a/httpcore-nio/src/main/java/org/apache/http/nio/protocol/HttpAsyncService.java b/httpcore-nio/src/main/java/org/apache/http/nio/protocol/HttpAsyncService.java
index ebeed8b..4d9b881 100644
--- a/httpcore-nio/src/main/java/org/apache/http/nio/protocol/HttpAsyncService.java
+++ b/httpcore-nio/src/main/java/org/apache/http/nio/protocol/HttpAsyncService.java
@@ -28,6 +28,7 @@
package org.apache.http.nio.protocol;
import java.io.IOException;
+import java.net.SocketException;
import java.net.SocketTimeoutException;
import java.util.Queue;
import java.util.concurrent.ConcurrentLinkedQueue;
@@ -285,39 +286,58 @@ public class HttpAsyncService implements NHttpServerEventHandler {
}
state.setTerminated();
closeHandlers(state, cause);
- final Cancellable cancellable = state.getCancellable();
- if (cancellable != null) {
- cancellable.cancel();
- }
- final Queue<PipelineEntry> pipeline = state.getPipeline();
- if (!pipeline.isEmpty()
- || conn.isResponseSubmitted()
- || state.getResponseState().compareTo(MessageState.INIT) > 0) {
- // There is not much that we can do if a response
- // has already been submitted or pipelining is being used.
- shutdownConnection(conn);
- } else {
- try {
- final Incoming incoming = state.getIncoming();
- final HttpRequest request = incoming != null ? incoming.getRequest() : null;
- final HttpContext context = incoming != null ? incoming.getContext() : new BasicHttpContext();
+ try {
+ final Cancellable cancellable = state.getCancellable();
+ if (cancellable != null) {
+ cancellable.cancel();
+ }
+ if (cause instanceof SocketException) {
+ // Transport layer is likely unreliable.
+ conn.shutdown();
+ return;
+ }
+ if (cause instanceof SocketTimeoutException) {
+ // Connection timed out due to inactivity.
+ conn.close();
+ return;
+ }
+
+ if (!state.getPipeline().isEmpty()
+ || conn.isResponseSubmitted()
+ || state.getResponseState().compareTo(MessageState.INIT) > 0) {
+ // There is not much that we can do if a response
+ // has already been submitted or pipelining is being used.
+ conn.close();
+ return;
+ }
+ final HttpRequest request = conn.getHttpRequest();
+ if (request != null) {
+ // Request head received but failed processing likely due to a protocol violation
+ conn.resetInput();
+ final HttpCoreContext context = HttpCoreContext.create();
final HttpAsyncResponseProducer responseProducer = handleException(cause, context);
final HttpResponse response = responseProducer.generateResponse();
final Outgoing outgoing = new Outgoing(request, response, responseProducer, context);
state.setResponseState(MessageState.INIT);
state.setOutgoing(outgoing);
commitFinalResponse(conn, state);
- } catch (final Exception ex) {
- shutdownConnection(conn);
- closeHandlers(state);
- if (ex instanceof RuntimeException) {
- throw (RuntimeException) ex;
- }
- log(ex);
+ return;
}
+ conn.close();
+ } catch (final Exception ex) {
+ shutdownConnection(conn);
+ closeHandlers(state);
+ if (ex instanceof RuntimeException) {
+ throw (RuntimeException) ex;
+ }
+ log(ex);
}
}
+ protected HttpResponse createHttpResponse(final int status, final HttpContext context) {
+ return this.responseFactory.newHttpResponse(HttpVersion.HTTP_1_1, status, context);
+ }
+
@Override
public void requestReceived(
final NHttpServerConnection conn) throws IOException, HttpException {
@@ -350,8 +370,7 @@ public class HttpAsyncService implements NHttpServerEventHandler {
&& !(conn instanceof SessionBufferStatus && ((SessionBufferStatus) conn).hasBufferedInput())) {
state.setRequestState(MessageState.ACK_EXPECTED);
- final HttpResponse ack = this.responseFactory.newHttpResponse(HttpVersion.HTTP_1_1,
- HttpStatus.SC_CONTINUE, context);
+ final HttpResponse ack = createHttpResponse(HttpStatus.SC_CONTINUE, context);
if (this.expectationVerifier != null) {
conn.suspendInput();
conn.suspendOutput();
@@ -443,8 +462,7 @@ public class HttpAsyncService implements NHttpServerEventHandler {
final HttpRequest request = pipelineEntry.getRequest();
final HttpContext context = pipelineEntry.getContext();
if (result != null) {
- final HttpResponse response = this.responseFactory.newHttpResponse(HttpVersion.HTTP_1_1,
- HttpStatus.SC_OK, context);
+ final HttpResponse response = createHttpResponse(HttpStatus.SC_OK, context);
final HttpAsyncExchangeImpl httpExchange = new HttpAsyncExchangeImpl(
request, response, state, conn, context);
final HttpAsyncRequestHandler<Object> handler = pipelineEntry.getHandler();
@@ -454,14 +472,7 @@ public class HttpAsyncService implements NHttpServerEventHandler {
} catch (final RuntimeException ex) {
throw ex;
} catch (final Exception ex) {
- pipeline.add(new PipelineEntry(
- request,
- null,
- ex,
- handler,
- context));
- state.setResponseState(MessageState.READY);
- responseReady(conn);
+ httpExchange.submitResponse(handleException(ex, context));
return;
}
} else {
@@ -529,7 +540,7 @@ public class HttpAsyncService implements NHttpServerEventHandler {
public void timeout(final NHttpServerConnection conn) throws IOException {
final State state = getState(conn);
if (state != null) {
- exception(conn, new SocketTimeoutException(
+ closeHandlers(state, new SocketTimeoutException(
String.format("%,d milliseconds timeout on connection %s", conn.getSocketTimeout(), conn)));
}
if (conn.getStatus() == NHttpConnection.ACTIVE) {
@@ -623,10 +634,8 @@ public class HttpAsyncService implements NHttpServerEventHandler {
if (message == null) {
message = ex.toString();
}
- final HttpResponse response = this.responseFactory.newHttpResponse(HttpVersion.HTTP_1_1,
- toStatusCode(ex, context), context);
- return new ErrorResponseProducer(response,
- new NStringEntity(message, ContentType.DEFAULT_TEXT), false);
+ final HttpResponse response = createHttpResponse(toStatusCode(ex, context), context);
+ return new ErrorResponseProducer(response, new NStringEntity(message, ContentType.DEFAULT_TEXT), false);
}
protected int toStatusCode(final Exception ex, final HttpContext context) {
@@ -637,8 +646,6 @@ public class HttpAsyncService implements NHttpServerEventHandler {
code = HttpStatus.SC_HTTP_VERSION_NOT_SUPPORTED;
} else if (ex instanceof ProtocolException) {
code = HttpStatus.SC_BAD_REQUEST;
- } else if (ex instanceof SocketTimeoutException) {
- code = HttpStatus.SC_GATEWAY_TIMEOUT;
} else {
code = HttpStatus.SC_INTERNAL_SERVER_ERROR;
}
diff --git a/httpcore-nio/src/test/java/org/apache/http/nio/protocol/TestHttpAsyncService.java b/httpcore-nio/src/test/java/org/apache/http/nio/protocol/TestHttpAsyncService.java
index dcb92a6..8ab870c 100644
--- a/httpcore-nio/src/test/java/org/apache/http/nio/protocol/TestHttpAsyncService.java
+++ b/httpcore-nio/src/test/java/org/apache/http/nio/protocol/TestHttpAsyncService.java
@@ -163,6 +163,7 @@ public class TestHttpAsyncService {
state.setIncoming(incoming);
state.setCancellable(this.cancellable);
this.connContext.setAttribute(HttpAsyncService.HTTP_EXCHANGE_STATE, state);
+ Mockito.when(this.conn.getHttpRequest()).thenReturn(request);
final HttpException httpex = new HttpException();
this.protocolHandler.exception(this.conn, httpex);
@@ -210,6 +211,7 @@ public class TestHttpAsyncService {
state.setOutgoing(outgoing);
state.setCancellable(this.cancellable);
this.connContext.setAttribute(HttpAsyncService.HTTP_EXCHANGE_STATE, state);
+ Mockito.when(this.conn.getHttpRequest()).thenReturn(request);
Mockito.doThrow(new RuntimeException()).when(this.httpProcessor).process(
Matchers.any(HttpResponse.class), Matchers.any(HttpContext.class));
@@ -243,6 +245,7 @@ public class TestHttpAsyncService {
state.setOutgoing(outgoing);
state.setCancellable(this.cancellable);
this.connContext.setAttribute(HttpAsyncService.HTTP_EXCHANGE_STATE, state);
+ Mockito.when(this.conn.getHttpRequest()).thenReturn(request);
Mockito.doThrow(new IOException()).when(this.httpProcessor).process(
Matchers.any(HttpResponse.class), Matchers.any(HttpContext.class));
@@ -280,7 +283,7 @@ public class TestHttpAsyncService {
Assert.assertEquals(MessageState.READY, state.getRequestState());
Assert.assertEquals(MessageState.READY, state.getResponseState());
- Mockito.verify(this.conn).shutdown();
+ Mockito.verify(this.conn).close();
Mockito.verify(this.requestConsumer).failed(httpex);
Mockito.verify(this.requestConsumer).close();
Mockito.verify(this.responseProducer).failed(httpex);
@@ -1329,19 +1332,31 @@ public class TestHttpAsyncService {
@Test
public void testTimeoutActiveConnection() throws Exception {
final State state = new State();
+ final HttpContext exchangeContext = new BasicHttpContext();
+ final HttpRequest request = new BasicHttpRequest("GET", "/", HttpVersion.HTTP_1_1);
+ final Incoming incoming = new Incoming(
+ request, this.requestHandler, this.requestConsumer, exchangeContext);
+ state.setIncoming(incoming);
this.connContext.setAttribute(HttpAsyncService.HTTP_EXCHANGE_STATE, state);
+ Mockito.when(this.conn.getHttpRequest()).thenReturn(request);
Mockito.when(this.conn.getStatus()).thenReturn(NHttpConnection.ACTIVE, NHttpConnection.CLOSED);
this.protocolHandler.timeout(this.conn);
- Mockito.verify(this.conn).close();
+ Mockito.verify(this.conn, Mockito.atLeastOnce()).close();
Mockito.verify(this.conn, Mockito.never()).setSocketTimeout(Matchers.anyInt());
}
@Test
public void testTimeoutActiveConnectionBufferedData() throws Exception {
final State state = new State();
+ final HttpContext exchangeContext = new BasicHttpContext();
+ final HttpRequest request = new BasicHttpRequest("GET", "/", HttpVersion.HTTP_1_1);
+ final Incoming incoming = new Incoming(
+ request, this.requestHandler, this.requestConsumer, exchangeContext);
+ state.setIncoming(incoming);
this.connContext.setAttribute(HttpAsyncService.HTTP_EXCHANGE_STATE, state);
+ Mockito.when(this.conn.getHttpRequest()).thenReturn(request);
Mockito.when(this.conn.getStatus()).thenReturn(NHttpConnection.ACTIVE, NHttpConnection.CLOSING);
this.protocolHandler.timeout(this.conn);