Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for SizeLimitHandlerTest #313

Closed
wants to merge 6 commits into from
Closed

Fixes for SizeLimitHandlerTest #313

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ea11c33
merge SizeLimitHandlerTest and SizeLimitIgnoreTest
lachlan-roberts Nov 25, 2024
2fa6684
fix sizeLimitHandler with Jetty 9.4 HttpMode
lachlan-roberts Nov 26, 2024
ef045b1
update to 12.0.15
lachlan-roberts Nov 26, 2024
27343ba
Merge remote-tracking branch 'origin/main' into sizeLimitHandlerTest
lachlan-roberts Dec 3, 2024
9b2e426
revert to 12.0.14
lachlan-roberts Dec 3, 2024
16e0097
Fix issues with null cause of exception in JettyHttpHandler
lachlan-roberts Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
<maven.compiler.target>1.8</maven.compiler.target>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<jetty.version>9.4.56.v20240826</jetty.version>
<jetty12.version>12.0.14</jetty12.version>
<jetty12.version>12.0.15</jetty12.version>
<io.grpc>1.68.1</io.grpc>
<io.netty>4.1.115.Final</io.netty>
<slf4j.version>2.0.16</slf4j.version>
Expand Down
36 changes: 34 additions & 2 deletions ...time_impl_jetty9/src/main/java/com/google/apphosting/runtime/jetty9/JettyHttpHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,10 @@
import java.time.Duration;
import java.util.concurrent.TimeoutException;
import javax.servlet.ServletException;
import javax.servlet.UnavailableException;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import org.eclipse.jetty.http.BadMessageException;
import org.eclipse.jetty.server.Handler;
import org.eclipse.jetty.server.HttpChannel;
import org.eclipse.jetty.server.Request;
Expand Down Expand Up @@ -137,8 +139,21 @@ public void handle(
// We will report the exception via the rpc. We don't mark this thread as interrupted because
// ThreadGroupPool would use that as a signal to remove the thread from the pool; we don't
// need that.
handleException(ex, requestToken, genericResponse);
response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, ex.getMessage());
Throwable cause = unwrap(ex, BadMessageException.class, UnavailableException.class);
handleException(cause, requestToken, genericResponse);
if (cause instanceof BadMessageException) {
BadMessageException bme = (BadMessageException) cause;
response.sendError(bme.getCode(), cause.getMessage());
} else if (cause instanceof UnavailableException) {
UnavailableException ue = (UnavailableException) cause;
response.sendError(
ue.isPermanent()
? HttpServletResponse.SC_NOT_FOUND
: HttpServletResponse.SC_SERVICE_UNAVAILABLE,
cause.getMessage());
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious on

  1. What response code do we give in Jetty 12? Also the RPC Connector mode?
  2. I see that we are returning 404 but the PR mentions 413

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UnavailableException has nothing to do with the SizeLimitHandlerTest, only the BadMessageException does. Previously in this method we were just sending 500 response, but the response really depends on the exception, so in case of a BadMessageException we should send a 400 and with a UnavailableException we send either 404 or 503 depending if its permanently unavailable.

The Jetty 12 runtime does the same thing just through a different mechanism, it will not even reach this point, it handles it internal to the ServletChannel and writes the error itself with the correct code. In the case that it did reach this point in the Jetty 12 runtime (by failing the callback), then it uses Response.writeError which will deal with HttpException to send a correct response code.

response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, cause.getMessage());
}
} finally {
// We don't want threads used for background requests to go back
// in the thread pool, because users may have stashed references
Expand Down Expand Up @@ -265,6 +280,23 @@ private void handleException(
setFailure(response, error, "Unexpected exception from servlet: " + ex);
}

/**
* Unwrap failure causes to find target class
*
* @param failure The throwable to have its causes unwrapped
* @param targets Exception classes that we should not unwrap
* @return A target throwable or null
*/
protected Throwable unwrap(Throwable failure, Class<?>... targets) {
while (failure != null) {
for (Class<?> x : targets) {
if (x.isInstance(failure)) return failure;
}
failure = failure.getCause();
}
return null;
}

/** Create a failure response from the given code and message. */
public static void setFailure(
ResponseAPIData response, RuntimePb.UPResponse.ERROR error, String message) {
Expand Down
133 changes: 76 additions & 57 deletions runtime/test/src/test/java/com/google/apphosting/runtime/jetty9/SizeLimitHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,14 @@
public class SizeLimitHandlerTest extends JavaRuntimeViaHttpBase {

@Parameterized.Parameters
public static Collection<Object[]> data() {
public static Collection<Object[]> parameters() {
return Arrays.asList(
new Object[][] {
{"jetty94", false},
{"jetty94", true},
{"ee8", false},
{"ee10", false},
{"ee8", true},
{"ee10", false},
{"ee10", true},
});
}
Expand All @@ -86,7 +87,7 @@ public SizeLimitHandlerTest(String environment, boolean httpMode) {
}

@Before
public void before() throws Exception {
public void start() throws Exception {
String app = "sizelimit" + environment;
copyAppToDir(app, temp.getRoot().toPath());
httpClient.start();
Expand All @@ -96,10 +97,11 @@ public void before() throws Exception {
}

@After
public void after() throws Exception
{
public void after() throws Exception {
httpClient.stop();
runtime.close();
if (runtime != null) {
runtime.close();
}
}

@Test
Expand All @@ -108,10 +110,15 @@ public void testResponseContentBelowMaxLength() throws Exception {
String url = runtime.jettyUrl("/?size=" + contentLength);
CompletableFuture<Result> completionListener = new CompletableFuture<>();
AtomicLong contentReceived = new AtomicLong();
httpClient.newRequest(url).onResponseContentAsync((response, content, callback) -> {
contentReceived.addAndGet(content.remaining());
callback.succeeded();
}).header("setCustomHeader", "true").send(completionListener::complete);
httpClient
.newRequest(url)
.onResponseContentAsync(
(response, content, callback) -> {
contentReceived.addAndGet(content.remaining());
callback.succeeded();
})
.header("setCustomHeader", "true")
.send(completionListener::complete);

Result result = completionListener.get(5, TimeUnit.SECONDS);
assertThat(result.getResponse().getStatus(), equalTo(HttpStatus.OK_200));
Expand Down Expand Up @@ -148,8 +155,9 @@ public void testResponseContentAboveMaxLength() throws Exception {
assertThat(received.length(), lessThanOrEqualTo(MAX_SIZE));

// No content is sent on the Jetty 9.4 runtime.
if (!"jetty94".equals(environment) && !httpMode)
if (!"jetty94".equals(environment) && !httpMode) {
assertThat(received.toString(), containsString("Response body is too large"));
}
}

@Test
Expand All @@ -159,14 +167,15 @@ public void testResponseContentBelowMaxLengthGzip() throws Exception {
CompletableFuture<Result> completionListener = new CompletableFuture<>();
AtomicLong contentReceived = new AtomicLong();
httpClient.getContentDecoderFactories().clear();
httpClient.newRequest(url)
.onResponseContentAsync((response, content, callback) ->
{
contentReceived.addAndGet(content.remaining());
callback.succeeded();
})
.header(HttpHeader.ACCEPT_ENCODING, "gzip")
.send(completionListener::complete);
httpClient
.newRequest(url)
.onResponseContentAsync(
(response, content, callback) -> {
contentReceived.addAndGet(content.remaining());
callback.succeeded();
})
.header(HttpHeader.ACCEPT_ENCODING, "gzip")
.send(completionListener::complete);

Result result = completionListener.get(5, TimeUnit.SECONDS);
assertThat(result.getResponse().getHeaders().get(HttpHeader.CONTENT_ENCODING), equalTo("gzip"));
Expand Down Expand Up @@ -209,16 +218,17 @@ public void testResponseContentAboveMaxLengthGzip() throws Exception {
assertThat(received.length(), lessThanOrEqualTo(MAX_SIZE));

// No content is sent on the Jetty 9.4 runtime.
if (!"jetty94".equals(environment) && !httpMode)
if (!"jetty94".equals(environment) && !httpMode) {
assertThat(received.toString(), containsString("Response body is too large"));
}
}

@Test
public void testRequestContentBelowMaxLength() throws Exception {
int contentLength = MAX_SIZE;

byte[] data = new byte[contentLength];
Arrays.fill(data, (byte)'X');
Arrays.fill(data, (byte) 'X');
ContentProvider content = new ByteBufferContentProvider(BufferUtil.toBuffer(data));
String url = runtime.jettyUrl("/");
ContentResponse response = httpClient.newRequest(url).content(content).send();
Expand All @@ -234,17 +244,19 @@ public void testRequestContentAboveMaxLength() throws Exception {

CompletableFuture<Result> completionListener = new CompletableFuture<>();
byte[] data = new byte[contentLength];
Arrays.fill(data, (byte)'X');
Arrays.fill(data, (byte) 'X');
Utf8StringBuilder received = new Utf8StringBuilder();
ContentProvider content = new ByteBufferContentProvider(BufferUtil.toBuffer(data));
String url = runtime.jettyUrl("/");
httpClient.newRequest(url).content(content)
.onResponseContentAsync((response, content1, callback) ->
{
received.append(content1);
callback.succeeded();
})
.send(completionListener::complete);
httpClient
.newRequest(url)
.content(content)
.onResponseContentAsync(
(response, content1, callback) -> {
received.append(content1);
callback.succeeded();
})
.send(completionListener::complete);

Result result = completionListener.get(5, TimeUnit.SECONDS);
assertThat(result.getResponse().getStatus(), equalTo(HttpStatus.PAYLOAD_TOO_LARGE_413));
Expand All @@ -261,19 +273,21 @@ public void testRequestContentBelowMaxLengthGzip() throws Exception {

CompletableFuture<Result> completionListener = new CompletableFuture<>();
byte[] data = new byte[contentLength];
Arrays.fill(data, (byte)'X');
Arrays.fill(data, (byte) 'X');
Utf8StringBuilder received = new Utf8StringBuilder();
ContentProvider content = new InputStreamContentProvider(gzip(data));

String url = runtime.jettyUrl("/");
httpClient.newRequest(url).content(content)
.onResponseContentAsync((response, content1, callback) ->
{
received.append(content1);
callback.succeeded();
})
.header(HttpHeader.CONTENT_ENCODING, "gzip")
.send(completionListener::complete);
httpClient
.newRequest(url)
.content(content)
.onResponseContentAsync(
(response, content1, callback) -> {
received.append(content1);
callback.succeeded();
})
.header(HttpHeader.CONTENT_ENCODING, "gzip")
.send(completionListener::complete);

Result result = completionListener.get(5, TimeUnit.SECONDS);
assertThat(result.getResponse().getStatus(), equalTo(HttpStatus.OK_200));
Expand All @@ -286,19 +300,21 @@ public void testRequestContentAboveMaxLengthGzip() throws Exception {

CompletableFuture<Result> completionListener = new CompletableFuture<>();
byte[] data = new byte[contentLength];
Arrays.fill(data, (byte)'X');
Arrays.fill(data, (byte) 'X');
Utf8StringBuilder received = new Utf8StringBuilder();
ContentProvider content = new InputStreamContentProvider(gzip(data));

String url = runtime.jettyUrl("/");
httpClient.newRequest(url).content(content)
.onResponseContentAsync((response, content1, callback) ->
{
httpClient
.newRequest(url)
.content(content)
.onResponseContentAsync(
(response, content1, callback) -> {
received.append(content1);
callback.succeeded();
})
.header(HttpHeader.CONTENT_ENCODING, "gzip")
.send(completionListener::complete);
.header(HttpHeader.CONTENT_ENCODING, "gzip")
.send(completionListener::complete);

Result result = completionListener.get(5, TimeUnit.SECONDS);
assertThat(result.getResponse().getStatus(), equalTo(HttpStatus.PAYLOAD_TOO_LARGE_413));
Expand All @@ -319,8 +335,9 @@ public void testResponseContentLengthHeader() throws Exception {
assertThat(response.getStatus(), equalTo(HttpStatus.INTERNAL_SERVER_ERROR_500));

// No content is sent on the Jetty 9.4 runtime.
if (!"jetty94".equals(environment))
if (!"jetty94".equals(environment)) {
assertThat(response.getContentAsString(), containsString("Response body is too large"));
}
}

@Test
Expand All @@ -330,17 +347,18 @@ public void testRequestContentLengthHeader() throws Exception {
int contentLength = MAX_SIZE + 1;
String url = runtime.jettyUrl("/");
Utf8StringBuilder received = new Utf8StringBuilder();
httpClient.newRequest(url)
.header(HttpHeader.CONTENT_LENGTH, Long.toString(contentLength))
.header("foo", "bar")
.content(provider)
.onResponseContentAsync((response, content, callback) ->
{
httpClient
.newRequest(url)
.header(HttpHeader.CONTENT_LENGTH, Long.toString(contentLength))
.header("foo", "bar")
.content(provider)
.onResponseContentAsync(
(response, content, callback) -> {
received.append(content);
callback.succeeded();
provider.close();
})
.send(completionListener::complete);
.send(completionListener::complete);

Result result = completionListener.get(5, TimeUnit.SECONDS);
Response response = result.getResponse();
Expand All @@ -358,13 +376,14 @@ private RuntimeContext<?> runtimeContext() throws Exception {
return RuntimeContext.create(config);
}

private void assertEnvironment() throws Exception
{
private void assertEnvironment() throws Exception {
String match;
switch (environment)
{
switch (environment) {
case "jetty94":
match = "org.eclipse.jetty.server.Request";
match =
httpMode
? "com.google.apphosting.runtime.jetty9.JettyRequestAPIData"
: "org.eclipse.jetty.server.Request";
break;
case "ee8":
match = "org.eclipse.jetty.ee8";
Expand Down
Loading
Loading