Skip to content

Commit

Permalink
Merge pull request #4428 from eclipse/jetty-9.4.x-4427-httpclient_ret…
Browse files Browse the repository at this point in the history
…ried_request_duplicates_cookies

Fixes #4427 - Retried request duplicates cookies.
  • Loading branch information
sbordet authored Dec 19, 2019
2 parents d1d8f9a + 91239b0 commit af7cb94
Show file tree
Hide file tree
Showing 6 changed files with 218 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,12 @@ public void send(Request request, Response.CompleteListener listener)

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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -326,10 +326,10 @@ private void process()
}
}

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)
Expand All @@ -346,7 +346,7 @@ public boolean process(final Connection connection)
}
else
{
final Request request = exchange.getRequest();
Request request = exchange.getRequest();
Throwable cause = request.getAbortCause();
if (cause != null)
{
Expand All @@ -368,9 +368,13 @@ public boolean process(final Connection connection)
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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ public class HttpRequest implements Request
private BiFunction<Request, Request, Response.CompleteListener> pushListener;
private Supplier<HttpFields> trailers;
private Object tag;
private boolean normalized;

protected HttpRequest(HttpClient client, HttpConversation conversation, URI uri)
{
Expand Down Expand Up @@ -803,6 +804,23 @@ public Throwable getAbortCause()
return aborted.get();
}

/**
* <p>Marks this request as <em>normalized</em>.</p>
* <p>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.</p>
*
* @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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ public SendFailure(Throwable failure, boolean retry)
@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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -139,12 +139,17 @@ public boolean isClosed()
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()
{
Expand Down
Original file line number Diff line number Diff line change
@@ -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<Connection> 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);
}
}
}

0 comments on commit af7cb94

Please sign in to comment.