Skip to content

Commit

Permalink
Alternative Handler architecture
Browse files Browse the repository at this point in the history 
cleanups
  • Loading branch information
@gregw
gregw committed Dec 11, 2022
1 parent c6d691d commit 0ed443b
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 67 deletions.
4 changes: 2 additions & 2 deletions jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -266,8 +266,8 @@ else if (cause != null)
Request errorRequest = new ErrorProcessor.ErrorRequest(request, status, message, cause);
try
{
errorProcessor.process(errorRequest, response, callback);
return;
if (errorProcessor.process(errorRequest, response, callback))
return;
}
catch (Exception e)
{
Expand Down
45 changes: 13 additions & 32 deletions ...-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ThreadLimitHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,8 @@ public boolean process(Request request, Response response, Callback callback) th

// We accept the request and will always process it.
LimitedRequest limitedRequest = new LimitedRequest(remote, next, request, response, callback);
return limitedRequest.process();
limitedRequest.process();
return true;
}

private static void getAndClose(CompletableFuture<Closeable> cf)
Expand Down Expand Up @@ -282,10 +283,8 @@ private String getXForwardedFor(Request request)
return (comma >= 0) ? forwardedFor.substring(comma + 1).trim() : forwardedFor;
}

private static class LimitedRequest extends Request.Wrapper implements Runnable
private static class LimitedRequest extends Request.Wrapper
{
// TODO apply the thread limit to demand and write callbacks

private final Remote _remote;
private final Handler _handler;
private final LimitedResponse _response;
Expand Down Expand Up @@ -315,7 +314,7 @@ protected Callback getCallback()
return _callback;
}

protected boolean process() throws Exception
protected void process() throws Exception
{
CompletableFuture<Closeable> futurePermit = _remote.acquire();

Expand All @@ -324,32 +323,28 @@ protected boolean process() throws Exception
{
if (LOG.isDebugEnabled())
LOG.debug("Threadpermitted {}", _remote);
return process(futurePermit);
process(futurePermit);
}
else
{
if (LOG.isDebugEnabled())
LOG.debug("Threadlimited {}", _remote);
futurePermit.thenAccept(c -> process(futurePermit));
}

if (LOG.isDebugEnabled())
LOG.debug("Threadlimited {}", _remote);
futurePermit.thenAccept(c -> process(futurePermit));
return true;
}

protected boolean process(CompletableFuture<Closeable> futurePermit)
protected void process(CompletableFuture<Closeable> futurePermit)
{
Callback callback = Callback.from(_callback, () -> getAndClose(futurePermit));

try
{
if (!_handler.process(this, _response, callback))
{
getAndClose(futurePermit);
return false;
}
Response.writeError(this, _response, callback, 404);
}
catch (Throwable x)
{
callback.failed(x);
}
return true;
}

@Override
Expand Down Expand Up @@ -388,20 +383,6 @@ public void demand(Runnable onContent)

super.demand(permittedDemand);
}

@Override
public void run()
{
try
{
if (!process())
Response.writeError(getWrapped(), getResponse(), getCallback(), 404);
}
catch (Throwable t)
{
Response.writeError(getWrapped(), getResponse(), getCallback(), t);
}
}
}

private static class LimitedResponse extends Response.Wrapper
Expand Down
60 changes: 27 additions & 33 deletions ...core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/TryPathsHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,10 @@
import javax.net.ssl.SSLSocket;

import org.eclipse.jetty.http.HttpContent;
import org.eclipse.jetty.http.HttpHeader;
import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.http.HttpTester;
import org.eclipse.jetty.http.HttpURI;
import org.eclipse.jetty.http.ResourceHttpContentFactory;
import org.eclipse.jetty.http.pathmap.ServletPathSpec;
import org.eclipse.jetty.io.Content;
import org.eclipse.jetty.server.Handler;
import org.eclipse.jetty.server.Request;
import org.eclipse.jetty.server.Response;
Expand All @@ -46,7 +43,6 @@

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.startsWith;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
Expand Down Expand Up @@ -164,7 +160,7 @@ public boolean process(Request request, Response response, Callback callback)
}

@Test
public void testTryPathsWithPathMappings() throws Exception
public void testTryPathsAsync404() throws Exception
{
ResourceHandler resourceHandler = new ResourceHandler()
{
Expand All @@ -175,34 +171,42 @@ protected HttpContent.Factory newHttpContentFactory()
return new ResourceHttpContentFactory(ResourceFactory.of(getBaseResource()), getMimeTypes());
}
};
resourceHandler.setDirAllowed(false);

PathMappingsHandler pathMappingsHandler = new PathMappingsHandler();
pathMappingsHandler.addMapping(new ServletPathSpec("/"), resourceHandler);
pathMappingsHandler.addMapping(new ServletPathSpec("*.php"), new Handler.Processor()
{
@Override
public void doProcess(Request request, Response response, Callback callback)
{
response.setStatus(HttpStatus.OK_200);
response.getHeaders().put(HttpHeader.CONTENT_TYPE, "text/plain; charset=utf-8");
String message = "PHP: pathInContext=%s, query=%s".formatted(Request.getPathInContext(request), request.getHttpURI().getQuery());
Content.Sink.write(response, true, message, callback);
}
});
pathMappingsHandler.addMapping(new ServletPathSpec("/forward"), new Handler.Processor()
resourceHandler.setDirAllowed(false);
resourceHandler.setHandler(new Handler.Abstract()
{
@Override
public void doProcess(Request request, Response response, Callback callback)
public boolean process(Request request, Response response, Callback callback)
{
if (Request.getPathInContext(request).endsWith("/notFound"))
{
request.getComponents().getThreadPool().execute(() ->
{
try
{
Thread.sleep(100);
Response.writeError(request, response, callback, 404);
}
catch (InterruptedException e)
{
callback.failed(e);
}
});
return true;
}

if (!Request.getPathInContext(request).startsWith("/forward"))
return false;

assertThat(Request.getPathInContext(request), equalTo("/forward"));
assertThat(request.getHttpURI().getQuery(), equalTo("p=/last"));
response.setStatus(HttpStatus.NO_CONTENT_204);
callback.succeeded();
return true;
}
});

start(List.of("/maintenance.txt", "$path", "/forward?p=$path"), pathMappingsHandler);
start(List.of("/notFound", "/maintenance.txt", "$path", "/forward?p=$path"), resourceHandler);

try (SocketChannel channel = SocketChannel.open())
{
Expand All @@ -228,20 +232,10 @@ public void doProcess(Request request, Response response, Callback callback)
assertEquals(HttpStatus.OK_200, response.getStatus());
assertEquals("hello", response.getContent());

// Request an existing PHP file.
Files.writeString(rootPath.resolve("index.php"), "raw-php-contents", StandardOpenOption.CREATE);
request = HttpTester.newRequest();
request.setURI(CONTEXT_PATH + "/index.php");
channel.write(request.generate());
response = HttpTester.parseResponse(channel);
assertNotNull(response);
assertEquals(HttpStatus.OK_200, response.getStatus());
assertThat(response.getContent(), startsWith("PHP: pathInContext=/index.php"));

// Create the "maintenance" file, it should be served first.
path = "maintenance.txt";
Files.writeString(rootPath.resolve(path), "maintenance", StandardOpenOption.CREATE);
// Make a second request with any path, we should get the maintenance file.
// Make a third request with any path, we should get the maintenance file.
request = HttpTester.newRequest();
request.setURI(CONTEXT_PATH + "/whatever");
channel.write(request.generate());
Expand Down

0 comments on commit 0ed443b

Please sign in to comment.