#10226 handle review comments

Signed-off-by: Ludovic Orban <lorban@bitronix.be>
This commit is contained in:
Ludovic Orban 2023-08-25 14:28:33 +02:00
parent a4e33a019f
commit 159238471b
8 changed files with 57 additions and 72 deletions

View File

@ -85,9 +85,9 @@ public class HttpClientProxyProtocolTest
{
LifeCycle.stop(client);
LifeCycle.stop(server);
Set<ArrayByteBufferPool.Tracking.TrackingBuffer> serverLeaks = serverBufferPool.getLeaks();
Set<ArrayByteBufferPool.Tracking.Buffer> serverLeaks = serverBufferPool.getLeaks();
assertEquals(0, serverLeaks.size(), serverBufferPool.dumpLeaks());
Set<ArrayByteBufferPool.Tracking.TrackingBuffer> clientLeaks = clientBufferPool.getLeaks();
Set<ArrayByteBufferPool.Tracking.Buffer> clientLeaks = clientBufferPool.getLeaks();
assertEquals(0, clientLeaks.size(), clientBufferPool.dumpLeaks());
}

View File

@ -22,8 +22,6 @@
<argLine>
@{argLine} ${jetty.surefire.argLine}
--add-reads org.eclipse.jetty.fcgi.server=org.eclipse.jetty.logging
--add-reads org.eclipse.jetty.fcgi.server=java.management
--add-reads org.eclipse.jetty.fcgi.server=jdk.management
</argLine>
</configuration>
</plugin>

View File

@ -75,27 +75,9 @@ public abstract class AbstractHttpClientServerTest
try
{
if (serverBufferPool != null)
{
try
{
Awaitility.await().atMost(3, TimeUnit.SECONDS).until(() -> serverBufferPool.getLeaks().size(), Matchers.is(0));
}
catch (Exception e)
{
fail(e.getMessage() + "\n---\nServer Leaks: " + serverBufferPool.dumpLeaks() + "---\n");
}
}
assertNoLeaks(serverBufferPool, "\n---\nServer Leaks: " + serverBufferPool.dumpLeaks() + "---\n");
if (clientBufferPool != null)
{
try
{
Awaitility.await().atMost(3, TimeUnit.SECONDS).until(() -> clientBufferPool.getLeaks().size(), Matchers.is(0));
}
catch (Exception e)
{
fail(e.getMessage() + "\n---\nClient Leaks: " + clientBufferPool.dumpLeaks() + "---\n");
}
}
assertNoLeaks(clientBufferPool, "\n---\nClient Leaks: " + clientBufferPool.dumpLeaks() + "---\n");
}
finally
{
@ -103,4 +85,16 @@ public abstract class AbstractHttpClientServerTest
LifeCycle.stop(server);
}
}
private void assertNoLeaks(ArrayByteBufferPool.Tracking bufferPool, String msg)
{
try
{
Awaitility.await().atMost(3, TimeUnit.SECONDS).until(() -> bufferPool.getLeaks().size(), Matchers.is(0));
}
catch (Exception e)
{
fail(e.getMessage() + msg);
}
}
}

View File

@ -239,8 +239,7 @@ public class HttpClientTest extends AbstractHttpClientServerTest
Content.Sink.write(response, false, UTF_8.encode(paramValue1));
String paramValue2 = fields.getValue(paramName2);
assertEquals("", paramValue2);
Content.Sink.write(response, true, UTF_8.encode("empty"));
callback.succeeded();
Content.Sink.write(response, true, "empty", callback);
return true;
}
});
@ -274,8 +273,7 @@ public class HttpClientTest extends AbstractHttpClientServerTest
Content.Sink.write(response, false, UTF_8.encode(paramValue));
}
String paramValue2 = fields.getValue(paramName2);
Content.Sink.write(response, true, UTF_8.encode(paramValue2));
callback.succeeded();
Content.Sink.write(response, true, paramValue2, callback);
return true;
}
});
@ -771,8 +769,7 @@ public class HttpClientTest extends AbstractHttpClientServerTest
public boolean handle(org.eclipse.jetty.server.Request request, org.eclipse.jetty.server.Response response, Callback callback) throws Exception
{
Content.Sink.write(response, false, UTF_8.encode("A"));
Content.Sink.write(response, true, UTF_8.encode("B"));
callback.succeeded();
Content.Sink.write(response, true, "B", callback);
return true;
}
});

View File

@ -577,14 +577,14 @@ public class ArrayByteBufferPool implements ByteBufferPool, Dumpable
* <p>A variant of {@link ArrayByteBufferPool} that tracks buffer
* acquires/releases, useful to identify buffer leaks.</p>
* <p>Use {@link #getLeaks()} when the system is idle to get
* the {@link TrackingBuffer}s that have been leaked, which contain
* the {@link Buffer}s that have been leaked, which contain
* the stack trace information of where the buffer was acquired.</p>
*/
public static class Tracking extends ArrayByteBufferPool
{
private static final Logger LOG = LoggerFactory.getLogger(Tracking.class);
private final Set<TrackingBuffer> buffers = ConcurrentHashMap.newKeySet();
private final Set<Buffer> buffers = ConcurrentHashMap.newKeySet();
public Tracking()
{
@ -605,14 +605,14 @@ public class ArrayByteBufferPool implements ByteBufferPool, Dumpable
public RetainableByteBuffer acquire(int size, boolean direct)
{
RetainableByteBuffer buffer = super.acquire(size, direct);
TrackingBuffer wrapper = new TrackingBuffer(buffer, size);
Buffer wrapper = new Buffer(buffer, size);
if (LOG.isDebugEnabled())
LOG.debug("acquired {}", wrapper);
buffers.add(wrapper);
return wrapper;
}
public Set<TrackingBuffer> getLeaks()
public Set<Buffer> getLeaks()
{
return buffers;
}
@ -620,11 +620,11 @@ public class ArrayByteBufferPool implements ByteBufferPool, Dumpable
public String dumpLeaks()
{
return getLeaks().stream()
.map(TrackingBuffer::dump)
.map(Buffer::dump)
.collect(Collectors.joining(System.lineSeparator()));
}
public class TrackingBuffer extends RetainableByteBuffer.Wrapper
public class Buffer extends RetainableByteBuffer.Wrapper
{
private final int size;
private final Instant acquireInstant;
@ -632,7 +632,7 @@ public class ArrayByteBufferPool implements ByteBufferPool, Dumpable
private final List<Throwable> retainStacks = new CopyOnWriteArrayList<>();
private final List<Throwable> releaseStacks = new CopyOnWriteArrayList<>();
private TrackingBuffer(RetainableByteBuffer wrapped, int size)
private Buffer(RetainableByteBuffer wrapped, int size)
{
super(wrapped);
this.size = size;

View File

@ -14,12 +14,12 @@
package org.eclipse.jetty.server.handler;
import java.io.ByteArrayOutputStream;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.net.HttpURLConnection;
import java.net.URI;
import java.nio.charset.StandardCharsets;
import java.nio.file.Path;
import java.security.KeyStore;
import javax.net.ssl.HostnameVerifier;
import javax.net.ssl.HttpsURLConnection;
@ -52,6 +52,8 @@ public class DebugHandlerTest
private SSLContext sslContext;
private Server server;
private ArrayByteBufferPool.Tracking httpTrackingBufferPool;
private ArrayByteBufferPool.Tracking sslTrackingBufferPool;
private URI serverURI;
private URI secureServerURI;
@ -63,15 +65,17 @@ public class DebugHandlerTest
{
server = new Server();
ServerConnector httpConnector = new ServerConnector(server, null, null, new ArrayByteBufferPool.Tracking(), 1, 1, new HttpConnectionFactory());
httpTrackingBufferPool = new ArrayByteBufferPool.Tracking();
ServerConnector httpConnector = new ServerConnector(server, null, null, httpTrackingBufferPool, 1, 1, new HttpConnectionFactory());
httpConnector.setPort(0);
server.addConnector(httpConnector);
File keystorePath = MavenTestingUtils.getTestResourceFile("keystore.p12");
Path keystorePath = MavenTestingUtils.getTestResourcePath("keystore.p12");
SslContextFactory.Server sslContextFactory = new SslContextFactory.Server();
sslContextFactory.setKeyStorePath(keystorePath.getAbsolutePath());
sslContextFactory.setKeyStorePath(keystorePath.toAbsolutePath().toString());
sslContextFactory.setKeyStorePassword("storepwd");
ServerConnector sslConnector = new ServerConnector(server, null, null, new ArrayByteBufferPool.Tracking(), 1, 1,
sslTrackingBufferPool = new ArrayByteBufferPool.Tracking();
ServerConnector sslConnector = new ServerConnector(server, null, null, sslTrackingBufferPool, 1, 1,
AbstractConnectionFactory.getFactories(sslContextFactory, new HttpConnectionFactory()));
server.addConnector(sslConnector);
@ -128,10 +132,10 @@ public class DebugHandlerTest
@AfterEach
public void stopServer() throws Exception
{
ArrayByteBufferPool.Tracking byteBufferPool = (ArrayByteBufferPool.Tracking)server.getConnectors()[0].getByteBufferPool();
try
{
assertThat("Server Leaks: " + byteBufferPool.dumpLeaks(), byteBufferPool.getLeaks().size(), Matchers.is(0));
assertThat("Server HTTP Leaks: " + httpTrackingBufferPool.dumpLeaks(), httpTrackingBufferPool.getLeaks().size(), Matchers.is(0));
assertThat("Server SSL Leaks: " + sslTrackingBufferPool.dumpLeaks(), sslTrackingBufferPool.getLeaks().size(), Matchers.is(0));
}
finally
{

View File

@ -60,6 +60,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals;
public class ServerConnectorSslServerTest extends HttpServerTestBase
{
private SSLContext _sslContext;
private ArrayByteBufferPool.Tracking trackingBufferPool;
public ServerConnectorSslServerTest()
{
@ -73,10 +74,10 @@ public class ServerConnectorSslServerTest extends HttpServerTestBase
SslContextFactory.Server sslContextFactory = new SslContextFactory.Server();
sslContextFactory.setKeyStorePath(keystorePath);
sslContextFactory.setKeyStorePassword("storepwd");
ArrayByteBufferPool.Tracking pool = new ArrayByteBufferPool.Tracking();
trackingBufferPool = new ArrayByteBufferPool.Tracking();
HttpConnectionFactory httpConnectionFactory = new HttpConnectionFactory();
ServerConnector connector = new ServerConnector(_server, null, null, pool, 1, 1, AbstractConnectionFactory.getFactories(sslContextFactory, httpConnectionFactory));
ServerConnector connector = new ServerConnector(_server, null, null, trackingBufferPool, 1, 1, AbstractConnectionFactory.getFactories(sslContextFactory, httpConnectionFactory));
SecureRequestCustomizer secureRequestCustomer = new SecureRequestCustomizer();
secureRequestCustomer.setSslSessionAttribute("SSL_SESSION");
httpConnectionFactory.getHttpConfiguration().addCustomizer(secureRequestCustomer);
@ -110,8 +111,7 @@ public class ServerConnectorSslServerTest extends HttpServerTestBase
@AfterEach
public void dispose() throws Exception
{
ArrayByteBufferPool.Tracking byteBufferPool = (ArrayByteBufferPool.Tracking)_server.getConnectors()[0].getByteBufferPool();
assertThat("Server Leaks: " + byteBufferPool.dumpLeaks(), byteBufferPool.getLeaks().size(), Matchers.is(0));
assertThat("Server Leaks: " + trackingBufferPool.dumpLeaks(), trackingBufferPool.getLeaks().size(), Matchers.is(0));
}
@Override

View File

@ -126,31 +126,9 @@ public class AbstractTest
try
{
if (serverBufferPool != null && !isLeakTrackingDisabled(testInfo, "server"))
{
try
{
Awaitility.await().atMost(3, TimeUnit.SECONDS).until(() -> serverBufferPool.getLeaks().size(), Matchers.is(0));
}
catch (Exception e)
{
String className = testInfo.getTestClass().orElseThrow().getName();
dumpHeap("server-" + className);
fail(e.getMessage() + "\n---\nServer Leaks: " + serverBufferPool.dumpLeaks() + "---\n");
}
}
assertNoLeaks(serverBufferPool, testInfo, "server-", "\n---\nServer Leaks: " + serverBufferPool.dumpLeaks() + "---\n");
if (clientBufferPool != null && !isLeakTrackingDisabled(testInfo, "client"))
{
try
{
Awaitility.await().atMost(3, TimeUnit.SECONDS).until(() -> clientBufferPool.getLeaks().size(), Matchers.is(0));
}
catch (Exception e)
{
String className = testInfo.getTestClass().orElseThrow().getName();
dumpHeap("client-" + className);
fail(e.getMessage() + "\n---\nClient Leaks: " + clientBufferPool.dumpLeaks() + "---\n");
}
}
assertNoLeaks(clientBufferPool, testInfo, "client-", "\n---\nClient Leaks: " + clientBufferPool.dumpLeaks() + "---\n");
}
finally
{
@ -159,6 +137,20 @@ public class AbstractTest
}
}
private void assertNoLeaks(ArrayByteBufferPool.Tracking bufferPool, TestInfo testInfo, String prefix, String msg) throws Exception
{
try
{
Awaitility.await().atMost(3, TimeUnit.SECONDS).until(() -> bufferPool.getLeaks().size(), Matchers.is(0));
}
catch (Exception e)
{
String className = testInfo.getTestClass().orElseThrow().getName();
dumpHeap(prefix + className + msg);
fail(e.getMessage());
}
}
private static boolean isLeakTrackingDisabled(TestInfo testInfo, String tagSubValue)
{
String disableLeakTrackingTagValue = "DisableLeakTracking";