Skip to content

Commit 647ee8b

Browse files
sbordetsbordet
authored
Fixes StackOverflowError in redirect tests. (#13533)
A stack overflow was happening when a redirect was failed (for example, bad scheme of the redirect location). This was caused as a side effect of the changes done in #13381. HttpRedirector was registering a `Response.CompleteListener` that was notifying the original request/response listeners. This is necessary to notify failures before the redirect request is sent. But calling this listener after the redirect request is sent causes a stack overflow. The solution is to check whether the redirect request is already part of the conversation. If so, the notification of the listeners is performed by the `HttpClient` implementation. Otherwise, it must be explicitly done by HttpRedirector. Signed-off-by: Simone Bordet <[email protected]>
1 parent c4587e0 commit 647ee8b

File tree

6 files changed

+103
-12
lines changed

6 files changed

+103
-12
lines changed

‎jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -491,7 +491,7 @@ public Origin createOrigin(Request request, Origin.Protocol protocol)
491491
String scheme = request.getScheme();
492492
if (!HttpScheme.HTTP.is(scheme) && !HttpScheme.HTTPS.is(scheme) &&
493493
!HttpScheme.WS.is(scheme) && !HttpScheme.WSS.is(scheme))
494-
throw new IllegalArgumentException("Invalid protocol " + scheme);
494+
throw new IllegalArgumentException("Invalid URI scheme " + scheme);
495495
scheme = scheme.toLowerCase(Locale.ENGLISH);
496496
String host = request.getHost();
497497
host = host.toLowerCase(Locale.ENGLISH);

‎jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpRedirector.java‎

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -155,16 +155,26 @@ public Request redirect(Request request, Response response, Response.CompleteLis
155155
if (LOG.isDebugEnabled())
156156
LOG.debug("Redirecting to {} (Location: {})", newURI, location);
157157

158-
if (listener == null)
158+
Response.CompleteListener wrapper = result ->
159159
{
160-
listener = result ->
160+
if (listener != null)
161+
listener.onComplete(result);
162+
if (result.isFailed())
161163
{
162-
if (result.isFailed())
164+
// If the redirect request has already been added to the conversation,
165+
// failures will be notified to the original request by HttpClient, and
166+
// we must not do it, otherwise recursion happens and then StackOverflowError.
167+
// If the redirect request has not been added to the conversation yet,
168+
// we must notify the failure by calling fail(), like we do when we
169+
// cannot redirect (see the else branch below where we cannot redirect
170+
// because the Location header is missing).
171+
Request redirectRequest = result.getRequest();
172+
if (!((HttpRequest)request).getConversation().contains(redirectRequest))
163173
fail(result);
164-
};
165-
}
174+
}
175+
};
166176

167-
return redirect(request, response, listener, newURI);
177+
return redirect(request, response, wrapper, newURI);
168178
}
169179
else
170180
{
@@ -293,10 +303,10 @@ private URI sanitize(String location)
293303
String authority = matcher.group(3);
294304
String path = matcher.group(4);
295305
String query = matcher.group(5);
296-
if (query.length() == 0)
306+
if (query.isEmpty())
297307
query = null;
298308
String fragment = matcher.group(6);
299-
if (fragment.length() == 0)
309+
if (fragment.isEmpty())
300310
fragment = null;
301311
try
302312
{
@@ -390,6 +400,9 @@ private void fail(Result result)
390400

391401
private void fail(Request request, Throwable requestFailure, Response response, Throwable responseFailure)
392402
{
403+
// This method should only be called to fail the conversation
404+
// when the redirect request has not been sent.
405+
// See comment in #redirect(Request, Response, CompleteListener).
393406
HttpConversation conversation = ((HttpRequest)request).getConversation();
394407
conversation.updateResponseListeners(null);
395408
conversation.getResponseListeners().emitFailureComplete(new Result(request, requestFailure, response, responseFailure));

‎jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpConversation.java‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import org.eclipse.jetty.client.Response;
2323
import org.eclipse.jetty.util.Attributes;
2424
import org.eclipse.jetty.util.Promise;
25+
import org.eclipse.jetty.util.TypeUtil;
2526
import org.slf4j.Logger;
2627
import org.slf4j.LoggerFactory;
2728

@@ -141,6 +142,17 @@ public void updateResponseListeners(Response.Listener overrideListener)
141142
this.listeners = listeners;
142143
}
143144

145+
/**
146+
* <p>Whether the given request is in this conversation.</p>
147+
*
148+
* @param request the request to test
149+
* @return whether the given request is in this conversation
150+
*/
151+
public boolean contains(Request request)
152+
{
153+
return getExchanges().stream().anyMatch(exchange -> exchange.getRequest() == request);
154+
}
155+
144156
/**
145157
* <p>Returns the total timeout for the conversation.</p>
146158
* <p>The conversation total timeout is the total timeout
@@ -167,6 +179,6 @@ public void abort(Throwable cause, Promise<Boolean> promise)
167179
@Override
168180
public String toString()
169181
{
170-
return String.format("%s[%x]", HttpConversation.class.getSimpleName(), hashCode());
182+
return String.format("%s@%x[exchanges=%d]", TypeUtil.toShortName(getClass()), hashCode(), exchanges.size());
171183
}
172184
}

‎jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpRequest.java‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -764,15 +764,31 @@ public ContentResponse send() throws InterruptedException, TimeoutException, Exe
764764
@Override
765765
public void send(Response.CompleteListener listener)
766766
{
767+
Destination destination = resolveDestination(listener);
768+
if (destination == null)
769+
return;
767770
try
768771
{
769-
Destination destination = client.resolveDestination(this);
770772
destination.send(this, listener);
771773
}
772774
catch (Throwable x)
773775
{
776+
abort(x);
777+
}
778+
}
779+
780+
private Destination resolveDestination(Response.CompleteListener listener)
781+
{
782+
try
783+
{
784+
return client.resolveDestination(this);
785+
}
786+
catch (Throwable x)
787+
{
788+
// Must notify the listener, since it is not yet associated with this request.
774789
Result result = new Result(this, x, new HttpResponse(this), null);
775790
abort(x).thenRun(() -> ResponseListeners.notifyComplete(listener, result));
791+
return null;
776792
}
777793
}
778794

‎jetty-core/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientRedirectTest.java‎

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
import static org.junit.jupiter.api.Assertions.assertArrayEquals;
4848
import static org.junit.jupiter.api.Assertions.assertEquals;
4949
import static org.junit.jupiter.api.Assertions.assertFalse;
50+
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
5051
import static org.junit.jupiter.api.Assertions.assertNotNull;
5152
import static org.junit.jupiter.api.Assertions.assertThrows;
5253
import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -314,7 +315,7 @@ public void testRedirectWithWrongScheme(Scenario scenario) throws Exception
314315
protected void service(Request request, org.eclipse.jetty.server.Response response) throws Exception
315316
{
316317
response.setStatus(303);
317-
response.getHeaders().put("Location", "ssh://localhost:" + connector.getLocalPort() + "/path");
318+
response.getHeaders().put("Location", "ssh://localhost:" + connector.getLocalPort() + "/wrong/scheme");
318319
}
319320
});
320321

@@ -659,6 +660,40 @@ else if ("/three".equals(target))
659660
});
660661
}
661662

663+
@ParameterizedTest
664+
@ArgumentsSource(ScenarioProvider.class)
665+
public void testAbortRedirectRequest(Scenario scenario) throws Exception
666+
{
667+
start(scenario, new EmptyServerHandler()
668+
{
669+
@Override
670+
protected void service(Request request, org.eclipse.jetty.server.Response response) throws Throwable
671+
{
672+
String serverURI = scenario.getScheme() + "://localhost:" + connector.getLocalPort();
673+
String target = Request.getPathInContext(request);
674+
if ("/initial".equals(target))
675+
{
676+
response.setStatus(HttpStatus.SEE_OTHER_303);
677+
response.getHeaders().put(HttpHeader.LOCATION, serverURI + "/redirect");
678+
}
679+
}
680+
});
681+
682+
client.getRequestListeners().addBeginListener(r ->
683+
{
684+
if ("/redirect".equals(r.getPath()))
685+
r.abort(new HttpRequestException("aborted by test", r));
686+
});
687+
688+
ExecutionException failure = assertThrows(ExecutionException.class, () -> client.newRequest("localhost", connector.getLocalPort())
689+
.scheme(scenario.getScheme())
690+
.path("/initial")
691+
.timeout(5, TimeUnit.SECONDS)
692+
.send());
693+
694+
assertInstanceOf(HttpRequestException.class, failure.getCause());
695+
}
696+
662697
private void testSameMethodRedirect(final Scenario scenario, final HttpMethod method, int redirectCode) throws Exception
663698
{
664699
testMethodRedirect(scenario, method, method, redirectCode);

‎jetty-core/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTest.java‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,7 @@
9494
import static org.junit.jupiter.api.Assertions.assertArrayEquals;
9595
import static org.junit.jupiter.api.Assertions.assertEquals;
9696
import static org.junit.jupiter.api.Assertions.assertFalse;
97+
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
9798
import static org.junit.jupiter.api.Assertions.assertNotNull;
9899
import static org.junit.jupiter.api.Assertions.assertThrows;
99100
import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -2096,6 +2097,20 @@ protected void service(org.eclipse.jetty.server.Request request, org.eclipse.jet
20962097
.send());
20972098
}
20982099

2100+
@ParameterizedTest
2101+
@ArgumentsSource(ScenarioProvider.class)
2102+
public void testRequestWithWrongScheme(Scenario scenario) throws Exception
2103+
{
2104+
start(scenario, new EmptyServerHandler());
2105+
2106+
ExecutionException failure = assertThrows(ExecutionException.class, () -> client.newRequest("localhost", connector.getLocalPort())
2107+
.scheme("ssh")
2108+
.timeout(5, TimeUnit.SECONDS)
2109+
.send());
2110+
2111+
assertInstanceOf(IllegalArgumentException.class, failure.getCause());
2112+
}
2113+
20992114
private void assertCopyRequest(Request original)
21002115
{
21012116
Request copy = client.copyRequest(original, original.getURI());

0 commit comments

Comments
 (0)