Skip to content

Commit

Permalink
[MRESOLVER-455] Streaming bodies must be closed (#398)
Browse files Browse the repository at this point in the history 
And JDK transport did not close them in case of errors.

---

https://issues.apache.org/jira/browse/MRESOLVER-455
  • Loading branch information
@cstamas
cstamas committed Dec 14, 2023
1 parent 57bd1c2 commit d96c084
Show file tree
Hide file tree
Showing 12 changed files with 1,202 additions and 106 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/maven-verify.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,5 +28,5 @@ jobs:
with:
ff-run: false
ff-site-run: false
jdk-matrix: '[ "17", "21" ]'
jdk-matrix: '[ "21" ]'

2 changes: 1 addition & 1 deletion Jenkinsfile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,5 @@
* under the License.
*/

asfMavenTlpStdBuild( 'jdks' : [ "17", "21" ] )
asfMavenTlpStdBuild( 'jdks' : [ "21" ] )

34 changes: 30 additions & 4 deletions ...solver-test-http/src/main/java/org/eclipse/aether/internal/test/util/http/HttpServer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,7 @@
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import java.io.File;
import java.io.FileInputStream;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.*;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.Base64;
Expand Down Expand Up @@ -125,6 +122,8 @@ public enum ChecksumHeader {

private final AtomicInteger connectionsToClose = new AtomicInteger(0);

private final AtomicInteger serverErrorsBeforeWorks = new AtomicInteger(0);

private final List<LogEntry> logEntries = Collections.synchronizedList(new ArrayList<>());

public String getHost() {
Expand Down Expand Up @@ -245,13 +244,19 @@ public HttpServer setConnectionsToClose(int connectionsToClose) {
return this;
}

public HttpServer setServerErrorsBeforeWorks(int serverErrorsBeforeWorks) {
this.serverErrorsBeforeWorks.set(serverErrorsBeforeWorks);
return this;
}

public HttpServer start() throws Exception {
if (server != null) {
return this;
}

HandlerList handlers = new HandlerList();
handlers.addHandler(new ConnectionClosingHandler());
handlers.addHandler(new ServerErrorHandler());
handlers.addHandler(new LogHandler());
handlers.addHandler(new ProxyAuthHandler());
handlers.addHandler(new AuthHandler());
Expand Down Expand Up @@ -286,6 +291,17 @@ public void handle(String target, Request req, HttpServletRequest request, HttpS
}
}

private class ServerErrorHandler extends AbstractHandler {
@Override
public void handle(String target, Request req, HttpServletRequest request, HttpServletResponse response)
throws IOException {
if (serverErrorsBeforeWorks.getAndDecrement() > 0) {
response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
writeResponseBodyMessage(response, "Oops, come back later!");
}
}
}

private class LogHandler extends AbstractHandler {
@Override
public void handle(String target, Request req, HttpServletRequest request, HttpServletResponse response) {
Expand Down Expand Up @@ -326,18 +342,21 @@ public void handle(String target, Request req, HttpServletRequest request, HttpS

if (ExpectContinue.FAIL.equals(expectContinue) && request.getHeader(HttpHeader.EXPECT.asString()) != null) {
response.setStatus(HttpServletResponse.SC_EXPECTATION_FAILED);
writeResponseBodyMessage(response, "Expectation was set to fail");
return;
}

File file = new File(repoDir, path.substring(5));
if (HttpMethod.GET.is(req.getMethod()) || HttpMethod.HEAD.is(req.getMethod())) {
if (!file.isFile() || path.endsWith(URIUtil.SLASH)) {
response.setStatus(HttpServletResponse.SC_NOT_FOUND);
writeResponseBodyMessage(response, "Not found");
return;
}
long ifUnmodifiedSince = request.getDateHeader(HttpHeader.IF_UNMODIFIED_SINCE.asString());
if (ifUnmodifiedSince != -1L && file.lastModified() > ifUnmodifiedSince) {
response.setStatus(HttpServletResponse.SC_PRECONDITION_FAILED);
writeResponseBodyMessage(response, "Precondition failed");
return;
}
long offset = 0L;
Expand All @@ -348,6 +367,7 @@ public void handle(String target, Request req, HttpServletRequest request, HttpS
offset = Long.parseLong(m.group(1));
if (offset >= file.length()) {
response.setStatus(HttpServletResponse.SC_REQUESTED_RANGE_NOT_SATISFIABLE);
writeResponseBodyMessage(response, "Range not satisfiable");
return;
}
}
Expand Down Expand Up @@ -447,6 +467,12 @@ public void handle(String target, Request req, HttpServletRequest request, HttpS
}
}

private void writeResponseBodyMessage(HttpServletResponse response, String message) throws IOException {
try (OutputStream outputStream = response.getOutputStream()) {
outputStream.write(message.getBytes(StandardCharsets.UTF_8));
}
}

private class RedirectHandler extends AbstractHandler {
@Override
public void handle(String target, Request req, HttpServletRequest request, HttpServletResponse response) {
Expand Down
45 changes: 38 additions & 7 deletions ...st-http/src/main/java/org/eclipse/aether/internal/test/util/http/HttpTransporterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,10 @@

import org.eclipse.aether.ConfigurationProperties;
import org.eclipse.aether.DefaultRepositoryCache;
import org.eclipse.aether.DefaultRepositorySystemSession;
import org.eclipse.aether.DefaultSessionData;
import org.eclipse.aether.internal.test.util.TestFileUtils;
import org.eclipse.aether.internal.test.util.TestUtils;
import org.eclipse.aether.internal.test.util.TestLocalRepositoryManager;
import org.eclipse.aether.internal.test.util.TestRepositorySystemSession;
import org.eclipse.aether.repository.Authentication;
import org.eclipse.aether.repository.Proxy;
import org.eclipse.aether.repository.RemoteRepository;
Expand Down Expand Up @@ -84,12 +84,14 @@ public class HttpTransporterTest {

private final Supplier<HttpTransporterFactory> transporterFactorySupplier;

protected DefaultRepositorySystemSession session;
protected TestRepositorySystemSession session;

protected HttpTransporterFactory factory;

protected HttpTransporter transporter;

protected Runnable closer;

protected File repoDir;

protected HttpServer httpServer;
Expand Down Expand Up @@ -134,9 +136,11 @@ protected void newTransporter(String url) throws Exception {
transporter.close();
transporter = null;
}
// here we "simulate" onSessionClose
// TODO: in UTs currently we cannot do it, sort it out
session = new DefaultRepositorySystemSession(session);
if (closer != null) {
closer.run();
closer = null;
}
session = new TestRepositorySystemSession(session);
session.setData(new DefaultSessionData());
transporter = factory.newInstance(session, newRepo(url));
}
Expand All @@ -146,7 +150,8 @@ protected void newTransporter(String url) throws Exception {
@BeforeEach
protected void setUp(TestInfo testInfo) throws Exception {
System.out.println("=== " + testInfo.getDisplayName() + " ===");
session = TestUtils.newSession();
session = new TestRepositorySystemSession(h -> this.closer = h);
session.setLocalRepositoryManager(new TestLocalRepositoryManager());
factory = transporterFactorySupplier.get();
repoDir = TestFileUtils.createTempDir();
TestFileUtils.writeString(new File(repoDir, "file.txt"), "test");
Expand All @@ -167,6 +172,10 @@ protected void tearDown() throws Exception {
transporter.close();
transporter = null;
}
if (closer != null) {
closer.run();
closer = null;
}
if (httpServer != null) {
httpServer.stop();
httpServer = null;
Expand Down Expand Up @@ -454,6 +463,28 @@ protected void testGet_SSL() throws Exception {
assertEquals(task.getDataString(), listener.getBaos().toString(StandardCharsets.UTF_8));
}

@Test
protected void testGet_SSL_WithServerErrors() throws Exception {
httpServer.setServerErrorsBeforeWorks(1);
httpServer.addSslConnector();
newTransporter(httpServer.getHttpsUrl());
for (int i = 1; i < 3; i++) {
try {
RecordingTransportListener listener = new RecordingTransportListener();
GetTask task = new GetTask(URI.create("repo/file.txt")).setListener(listener);
transporter.get(task);
assertEquals("test", task.getDataString());
assertEquals(0L, listener.getDataOffset());
assertEquals(4L, listener.getDataLength());
assertEquals(1, listener.getStartedCount());
assertTrue(listener.getProgressedCount() > 0, "Count: " + listener.getProgressedCount());
assertEquals(task.getDataString(), listener.getBaos().toString(StandardCharsets.UTF_8));
} catch (HttpTransporterException e) {
assertEquals(500, e.getStatusCode());
}
}
}

@Test
protected void testGet_HTTPS_Unknown_SecurityMode() throws Exception {
session.setConfigProperty(ConfigurationProperties.HTTPS_SECURITY_MODE, "unknown");
Expand Down

0 comments on commit d96c084

Please sign in to comment.