From 91239b01e3fdac2f183fa6c77c766d7134a4be54 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Tue, 17 Dec 2019 20:16:46 +0100 Subject: [PATCH] Fixes #4427 - Retried request duplicates cookies. Introduced HttpRequest.normalized() to test and set whether the request has already been normalized. Added test case and few cleanups. Signed-off-by: Simone Bordet --- .../eclipse/jetty/client/HttpConnection.java | 6 + .../eclipse/jetty/client/HttpDestination.java | 14 +- .../org/eclipse/jetty/client/HttpRequest.java | 18 ++ .../org/eclipse/jetty/client/SendFailure.java | 6 +- .../client/http/HttpConnectionOverHTTP.java | 7 +- .../client/HttpClientIdleTimeoutTest.java | 174 ++++++++++++++++++ 6 files changed, 218 insertions(+), 7 deletions(-) create mode 100644 jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientIdleTimeoutTest.java diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpConnection.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpConnection.java index 8e70a0353ca..bfa13a2ddf1 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpConnection.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpConnection.java @@ -84,6 +84,12 @@ public abstract class HttpConnection implements Connection protected void normalizeRequest(Request request) { + boolean normalized = ((HttpRequest)request).normalized(); + if (LOG.isDebugEnabled()) + LOG.debug("Normalizing {} {}", !normalized, request); + if (normalized) + return; + HttpVersion version = request.getVersion(); HttpFields headers = request.getHeaders(); ContentProvider content = request.getContent(); diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.java index 640dfc24d03..c42fcb6d8e9 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.java @@ -326,10 +326,10 @@ public abstract class HttpDestination extends ContainerLifeCycle implements Dest } } - public boolean process(final Connection connection) + public boolean process(Connection connection) { HttpClient client = getHttpClient(); - final HttpExchange exchange = getHttpExchanges().poll(); + HttpExchange exchange = getHttpExchanges().poll(); if (LOG.isDebugEnabled()) LOG.debug("Processing exchange {} on {} of {}", exchange, connection, this); if (exchange == null) @@ -346,7 +346,7 @@ public abstract class HttpDestination extends ContainerLifeCycle implements Dest } else { - final Request request = exchange.getRequest(); + Request request = exchange.getRequest(); Throwable cause = request.getAbortCause(); if (cause != null) { @@ -368,9 +368,13 @@ public abstract class HttpDestination extends ContainerLifeCycle implements Dest if (LOG.isDebugEnabled()) LOG.debug("Send failed {} for {}", result, exchange); if (result.retry) + { + // Resend this exchange, likely on another connection, + // and return false to avoid to re-enter this method. send(exchange); - else - request.abort(result.failure); + return false; + } + request.abort(result.failure); } } return getHttpExchanges().peek() != null; diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpRequest.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpRequest.java index 6a820a24119..87024b82f5a 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpRequest.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpRequest.java @@ -88,6 +88,7 @@ public class HttpRequest implements Request private BiFunction pushListener; private Supplier trailers; private Object tag; + private boolean normalized; protected HttpRequest(HttpClient client, HttpConversation conversation, URI uri) { @@ -803,6 +804,23 @@ public class HttpRequest implements Request return aborted.get(); } + /** + *

Marks this request as normalized.

+ *

A request is normalized by setting things that applications give + * for granted such as defaulting the method to {@code GET}, adding the + * {@code Host} header, adding the cookies, adding {@code Authorization} + * headers, etc.

+ * + * @return whether this request was already normalized + * @see HttpConnection#normalizeRequest(Request) + */ + boolean normalized() + { + boolean result = normalized; + normalized = true; + return result; + } + private String buildQuery() { StringBuilder result = new StringBuilder(); diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/SendFailure.java b/jetty-client/src/main/java/org/eclipse/jetty/client/SendFailure.java index 53df9eb3449..bade58b1461 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/SendFailure.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/SendFailure.java @@ -32,6 +32,10 @@ public class SendFailure @Override public String toString() { - return String.format("%s[failure=%s,retry=%b]", super.toString(), failure, retry); + return String.format("%s@%x[failure=%s,retry=%b]", + getClass().getSimpleName(), + hashCode(), + failure, + retry); } } diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpConnectionOverHTTP.java b/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpConnectionOverHTTP.java index c81530703ac..a90b2d3112e 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpConnectionOverHTTP.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpConnectionOverHTTP.java @@ -139,12 +139,17 @@ public class HttpConnectionOverHTTP extends AbstractConnection implements Connec public boolean onIdleExpired() { long idleTimeout = getEndPoint().getIdleTimeout(); - boolean close = delegate.onIdleTimeout(idleTimeout); + boolean close = onIdleTimeout(idleTimeout); if (close) close(new TimeoutException("Idle timeout " + idleTimeout + " ms")); return false; } + protected boolean onIdleTimeout(long idleTimeout) + { + return delegate.onIdleTimeout(idleTimeout); + } + @Override public void onFillable() { diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientIdleTimeoutTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientIdleTimeoutTest.java new file mode 100644 index 00000000000..c46d30577ab --- /dev/null +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientIdleTimeoutTest.java @@ -0,0 +1,174 @@ +// +// ======================================================================== +// Copyright (c) 1995-2019 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.client; + +import java.io.IOException; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import javax.servlet.ServletException; +import javax.servlet.http.Cookie; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.eclipse.jetty.client.api.Connection; +import org.eclipse.jetty.client.api.ContentResponse; +import org.eclipse.jetty.client.http.HttpClientTransportOverHTTP; +import org.eclipse.jetty.client.http.HttpConnectionOverHTTP; +import org.eclipse.jetty.client.http.HttpDestinationOverHTTP; +import org.eclipse.jetty.http.HttpStatus; +import org.eclipse.jetty.io.EndPoint; +import org.eclipse.jetty.server.Handler; +import org.eclipse.jetty.server.Request; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.server.ServerConnector; +import org.eclipse.jetty.util.Promise; +import org.eclipse.jetty.util.thread.QueuedThreadPool; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class HttpClientIdleTimeoutTest +{ + private Server server; + private ServerConnector connector; + private HttpClient client; + + private void start(Handler handler) throws Exception + { + QueuedThreadPool serverThreads = new QueuedThreadPool(); + serverThreads.setName("server"); + server = new Server(serverThreads); + connector = new ServerConnector(server, 1, 1); + server.addConnector(connector); + server.setHandler(handler); + server.start(); + } + + @AfterEach + public void dispose() throws Exception + { + if (server != null) + server.stop(); + if (client != null) + client.stop(); + } + + @Test + public void testRequestIsRetriedWhenSentDuringIdleTimeout() throws Exception + { + start(new EmptyServerHandler() + { + @Override + protected void service(String target, Request jettyRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + Cookie[] cookies = request.getCookies(); + if (cookies == null || cookies.length == 0) + { + // Send a cookie in the first response. + response.addCookie(new Cookie("name", "value")); + } + else + { + // Verify that there is only one cookie, i.e. + // that the request has not been normalized twice. + assertEquals(1, cookies.length); + } + } + }); + + CountDownLatch idleTimeoutLatch = new CountDownLatch(1); + CountDownLatch requestLatch = new CountDownLatch(1); + CountDownLatch retryLatch = new CountDownLatch(1); + QueuedThreadPool clientThreads = new QueuedThreadPool(); + clientThreads.setName("client"); + client = new HttpClient(new HttpClientTransportOverHTTP(1) + { + @Override + public HttpDestination newHttpDestination(Origin origin) + { + return new HttpDestinationOverHTTP(getHttpClient(), origin) + { + @Override + protected SendFailure send(Connection connection, HttpExchange exchange) + { + SendFailure result = super.send(connection, exchange); + if (result != null && result.retry) + retryLatch.countDown(); + return result; + } + }; + } + + @Override + protected HttpConnectionOverHTTP newHttpConnection(EndPoint endPoint, HttpDestination destination, Promise promise) + { + return new HttpConnectionOverHTTP(endPoint, destination, promise) + { + @Override + protected boolean onIdleTimeout(long idleTimeout) + { + boolean result = super.onIdleTimeout(idleTimeout); + if (result) + idleTimeoutLatch.countDown(); + assertTrue(await(requestLatch)); + return result; + } + }; + } + }, null); + client.setExecutor(clientThreads); + client.start(); + + long idleTimeout = 1000; + client.setIdleTimeout(idleTimeout); + + // Create one connection. + ContentResponse response = client.newRequest("localhost", connector.getLocalPort()).send(); + assertEquals(response.getStatus(), HttpStatus.OK_200); + + assertTrue(idleTimeoutLatch.await(2 * idleTimeout, TimeUnit.MILLISECONDS)); + + // Send a request exactly while the connection is idle timing out. + CountDownLatch responseLatch = new CountDownLatch(1); + client.newRequest("localhost", connector.getLocalPort()).send(result -> + { + assertTrue(result.isSucceeded()); + assertEquals(HttpStatus.OK_200, result.getResponse().getStatus()); + responseLatch.countDown(); + }); + assertTrue(retryLatch.await(5, TimeUnit.SECONDS)); + requestLatch.countDown(); + + assertTrue(responseLatch.await(5, TimeUnit.SECONDS)); + } + + private boolean await(CountDownLatch latch) + { + try + { + return latch.await(15, TimeUnit.SECONDS); + } + catch (InterruptedException x) + { + throw new RuntimeException(x); + } + } +}