Skip to content

Commit 906e3ff

Browse files
lorbanlorban
authored
12.0.x: Fix loop caused by NegotiatingClientConnection talking to broken server (#14132)
#13964 rework when connections are upgraded or failed Signed-off-by: Ludovic Orban <[email protected]>
1 parent 056c513 commit 906e3ff

File tree

2 files changed

+152
-40
lines changed

2 files changed

+152
-40
lines changed

‎jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/BrokenServerNegotiationTest.java‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
//
2+
// ========================================================================
3+
// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others.
4+
//
5+
// This program and the accompanying materials are made available under the
6+
// terms of the Eclipse Public License v. 2.0 which is available at
7+
// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
8+
// which is available at https://www.apache.org/licenses/LICENSE-2.0.
9+
//
10+
// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
11+
// ========================================================================
12+
//
13+
14+
package org.eclipse.jetty.http2.tests;
15+
16+
import java.net.ServerSocket;
17+
import java.net.Socket;
18+
import java.util.concurrent.ExecutionException;
19+
import java.util.concurrent.TimeUnit;
20+
import javax.net.ssl.SSLHandshakeException;
21+
22+
import org.eclipse.jetty.client.HttpClient;
23+
import org.eclipse.jetty.client.transport.HttpClientConnectionFactory;
24+
import org.eclipse.jetty.client.transport.HttpClientTransportDynamic;
25+
import org.eclipse.jetty.http2.client.HTTP2Client;
26+
import org.eclipse.jetty.http2.client.transport.ClientConnectionFactoryOverHTTP2;
27+
import org.eclipse.jetty.http2.client.transport.HttpClientTransportOverHTTP2;
28+
import org.eclipse.jetty.io.ClientConnectionFactory;
29+
import org.eclipse.jetty.io.ClientConnector;
30+
import org.eclipse.jetty.util.component.LifeCycle;
31+
import org.junit.jupiter.api.AfterEach;
32+
import org.junit.jupiter.params.ParameterizedTest;
33+
import org.junit.jupiter.params.provider.ValueSource;
34+
35+
import static org.awaitility.Awaitility.await;
36+
import static org.hamcrest.Matchers.notNullValue;
37+
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
38+
import static org.junit.jupiter.api.Assertions.assertThrows;
39+
40+
public class BrokenServerNegotiationTest
41+
{
42+
private volatile ServerSocket serverSocket;
43+
44+
public int startServer() throws Exception
45+
{
46+
if (serverSocket != null)
47+
return serverSocket.getLocalPort();
48+
Thread server = new Thread(() ->
49+
{
50+
try
51+
{
52+
serverSocket = new ServerSocket(0);
53+
while (true)
54+
{
55+
Socket accept = serverSocket.accept();
56+
accept.close();
57+
}
58+
}
59+
catch (Exception e)
60+
{
61+
// ignore, time to shut down
62+
}
63+
});
64+
server.start();
65+
await().atMost(5, TimeUnit.SECONDS).until(() -> serverSocket, notNullValue());
66+
return serverSocket.getLocalPort();
67+
}
68+
69+
@AfterEach
70+
public void stopServer() throws Exception
71+
{
72+
if (serverSocket != null)
73+
serverSocket.close();
74+
}
75+
76+
@ParameterizedTest
77+
@ValueSource(booleans = {false, true})
78+
public void testAlpnNegotiation(boolean useAlpn) throws Exception
79+
{
80+
int port = startServer();
81+
ClientConnector clientConnector = new ClientConnector();
82+
HTTP2Client h2Client = new HTTP2Client();
83+
h2Client.setUseALPN(useAlpn);
84+
ClientConnectionFactoryOverHTTP2.HTTP2 http2 = new ClientConnectionFactoryOverHTTP2.HTTP2(h2Client);
85+
ClientConnectionFactory.Info http1 = HttpClientConnectionFactory.HTTP11;
86+
HttpClientTransportDynamic transportDynamic = new HttpClientTransportDynamic(clientConnector, http1, http2);
87+
HttpClient client = new HttpClient(transportDynamic);
88+
client.start();
89+
90+
ExecutionException ee = assertThrows(ExecutionException.class, () -> client.GET("https://localhost:" + port + "/hello"));
91+
assertInstanceOf(SSLHandshakeException.class, ee.getCause());
92+
93+
LifeCycle.stop(client);
94+
}
95+
96+
@ParameterizedTest
97+
@ValueSource(booleans = {false, true})
98+
public void testH2Only(boolean useAlpn) throws Exception
99+
{
100+
int port = startServer();
101+
HTTP2Client h2Client = new HTTP2Client();
102+
h2Client.setUseALPN(useAlpn);
103+
HttpClientTransportOverHTTP2 httpClientTransportOverHTTP2 = new HttpClientTransportOverHTTP2(h2Client);
104+
HttpClient client = new HttpClient(httpClientTransportOverHTTP2);
105+
client.start();
106+
107+
ExecutionException ee = assertThrows(ExecutionException.class, () -> client.GET("https://localhost:" + port + "/hello"));
108+
assertInstanceOf(SSLHandshakeException.class, ee.getCause());
109+
110+
LifeCycle.stop(client);
111+
}
112+
}

‎jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/NegotiatingClientConnection.java‎

Lines changed: 40 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import java.util.concurrent.Executor;
1919
import java.util.concurrent.TimeoutException;
2020
import javax.net.ssl.SSLEngine;
21+
import javax.net.ssl.SSLHandshakeException;
2122

2223
import org.eclipse.jetty.util.BufferUtil;
2324
import org.eclipse.jetty.util.Promise;
@@ -55,7 +56,7 @@ public String getProtocol()
5556
protected void completed(String protocol)
5657
{
5758
this.protocol = protocol;
58-
completed = true;
59+
this.completed = true;
5960
}
6061

6162
@Override
@@ -73,67 +74,53 @@ public void onOpen()
7374
catch (Throwable x)
7475
{
7576
close();
76-
// TODO: should we not fail the promise in the context here?
77-
throw new RuntimeIOException(x);
77+
failConnectionPromise(x);
7878
}
7979
}
8080

8181
@Override
8282
public void onFillable()
8383
{
84-
while (true)
84+
Throwable failure = null;
85+
try
8586
{
86-
int filled = fill();
87-
if (completed || filled < 0)
87+
while (true)
8888
{
89-
replaceConnection();
90-
break;
91-
}
92-
if (filled == 0)
93-
{
94-
fillInterested();
95-
break;
89+
int filled = getEndPoint().fill(BufferUtil.EMPTY_BUFFER);
90+
if (completed)
91+
{
92+
replaceConnection();
93+
break;
94+
}
95+
else if (filled < 0)
96+
{
97+
throw new SSLHandshakeException("Abruptly closed by peer");
98+
}
99+
else if (filled == 0)
100+
{
101+
fillInterested();
102+
break;
103+
}
96104
}
97105
}
98-
}
99-
100-
private int fill()
101-
{
102-
try
103-
{
104-
return getEndPoint().fill(BufferUtil.EMPTY_BUFFER);
105-
}
106-
catch (IOException x)
106+
catch (Throwable x)
107107
{
108-
LOG.debug("Unable to fill from endpoint", x);
109-
close();
110-
return -1;
108+
failure = x;
111109
}
112-
}
113110

114-
private void replaceConnection()
115-
{
116-
EndPoint endPoint = getEndPoint();
117-
try
111+
if (failure != null)
118112
{
119-
endPoint.upgrade(connectionFactory.newConnection(endPoint, context));
120-
}
121-
catch (Throwable x)
122-
{
123-
LOG.debug("Unable to replace connection", x);
113+
LOG.atDebug().setCause(failure).log("Unable to fill from endpoint");
124114
close();
115+
failConnectionPromise(failure);
125116
}
126117
}
127118

128119
@Override
129120
public boolean onIdleExpired(TimeoutException timeout)
130121
{
131122
getEndPoint().close(timeout);
132-
133-
@SuppressWarnings("unchecked")
134-
Promise<Connection> promise = (Promise<Connection>)context.get(ClientConnector.CONNECTION_PROMISE_CONTEXT_KEY);
135-
promise.failed(timeout);
136-
123+
failConnectionPromise(timeout);
137124
return false;
138125
}
139126

@@ -144,4 +131,17 @@ public void close()
144131
getEndPoint().shutdownOutput();
145132
super.close();
146133
}
134+
135+
private void replaceConnection() throws IOException
136+
{
137+
EndPoint endPoint = getEndPoint();
138+
endPoint.upgrade(connectionFactory.newConnection(endPoint, context));
139+
}
140+
141+
private void failConnectionPromise(Throwable failure)
142+
{
143+
@SuppressWarnings("unchecked")
144+
Promise<Connection> promise = (Promise<Connection>)context.get(ClientConnector.CONNECTION_PROMISE_CONTEXT_KEY);
145+
promise.failed(failure);
146+
}
147147
}

0 commit comments

Comments
 (0)