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

Jump to bottom

cleanup of the JettyServletEngineAdapters and JettyHttpProxy for Jetty 9.4 and 12 #309

Merged
merged 5 commits into from
Nov 18, 2024
Merged

cleanup of the JettyServletEngineAdapters and JettyHttpProxy for Jetty 9.4 and 12 #309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6fac4e9
fix GzipHandler issues with HttpConnector mode enabled on Jetty 9.4
lachlan-roberts Nov 13, 2024
274ad9f
optimization for the Jetty 12 JettyRequestAPIData
lachlan-roberts Nov 13, 2024
fb24250
cleanup of the JettyServletEngineAdapters and JettyHttpProxy for Jett…
lachlan-roberts Nov 14, 2024
8e3083f
reformat JettyServletEngineAdapter with google style
lachlan-roberts Nov 15, 2024
98fb566
Merge remote-tracking branch 'origin/main' into cleanupOfJettyServlet…
lachlan-roberts Nov 18, 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
175 changes: 0 additions & 175 deletions ..._impl_jetty12/src/main/java/com/google/apphosting/runtime/jetty/CoreSizeLimitHandler.java
View file
Open in desktop

This file was deleted.

73 changes: 41 additions & 32 deletions ..._jetty12/src/main/java/com/google/apphosting/runtime/jetty/JettyServletEngineAdapter.java
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would that fix the flakiness seen with 12.0.15 #304 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't think so, I compared both SizeLimitHandlers and they were essentially the same other than some improved error handling in the Jetty one.

Opened #310 to track this.

Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
import org.eclipse.jetty.server.HttpConfiguration;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.ServerConnector;
import org.eclipse.jetty.server.SizeLimitHandler;
import org.eclipse.jetty.util.VirtualThreads;
import org.eclipse.jetty.util.resource.Resource;
import org.eclipse.jetty.util.resource.ResourceFactory;
Expand Down Expand Up @@ -102,6 +103,7 @@ private static AppYaml getAppYaml(ServletEngineAdapter.Config runtimeOptions) {

@Override
public void start(String serverInfo, ServletEngineAdapter.Config runtimeOptions) {
boolean isHttpConnectorMode = Boolean.getBoolean(HTTP_CONNECTOR_MODE);
QueuedThreadPool threadPool =
new QueuedThreadPool(MAX_THREAD_POOL_THREADS, MIN_THREAD_POOL_THREADS);
// Try to enable virtual threads if requested and on java21:
Expand All @@ -118,27 +120,44 @@ public InvocationType getInvocationType() {
return InvocationType.BLOCKING;
}
};
rpcConnector =
new DelegateConnector(server, "RPC") {
@Override
public void run(Runnable runnable) {
// Override this so that it does the initial run in the same thread.
// Currently, we block until completion in serviceRequest() so no point starting new
// thread.
runnable.run();
}
};
server.addConnector(rpcConnector);

// Don't add the RPC Connector if in HttpConnector mode.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hum. so we did that, what the side effect? 2 connectors and one not used? More memory used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably, but I don't think the memory saving here is that significant considering the DelegateConnector was never used.

if (!isHttpConnectorMode) {
rpcConnector =
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand that the rpcConnector won't be needed for HTTP mode but with this change we are no longer initializating this field anymore for both modes. Any chance that it would cause an NPE below for HttpConnector when we actually service the request?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This field is only ever used in JettyServletEngineAdaptor.serviceRequest, and in the GEN2 runtimes the only way we get to serviceRequest is through the ForwardingHandler in the JettyHttpProxy, and in HttpConnectorMode we never add this handler to go over RPC.

new DelegateConnector(server, "RPC") {
@Override
public void run(Runnable runnable) {
// Override this so that it does the initial run in the same thread.
// Currently, we block until completion in serviceRequest() so no point starting new
// thread.
runnable.run();
}
};

HttpConfiguration httpConfiguration = rpcConnector.getHttpConfiguration();
httpConfiguration.setSendDateHeader(false);
httpConfiguration.setSendServerVersion(false);
httpConfiguration.setSendXPoweredBy(false);
if (LEGACY_MODE) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a question there (hard to diff), LEGACY_MODE is still in both rpc and http connector, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes but for http connector it is done in com.google.apphosting.runtime.jetty.proxy.JettyHttpProxy#newConnector.

httpConfiguration.setRequestCookieCompliance(CookieCompliance.RFC2965);
httpConfiguration.setResponseCookieCompliance(CookieCompliance.RFC2965);
httpConfiguration.setUriCompliance(UriCompliance.LEGACY);
}

Comment on lines +137 to +146
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are your thoughts about movign all this to the constructor of DelegateConnector itself? I see that it class is only here and if you feel you don't want to give these defaults for all constructors of this instance, should we create static createDefaultDelegateConnector() ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What are your thoughts about movign all this to the constructor of DelegateConnector itself?

I wrote the jetty-delegate code to be a generic mechansim not google specific things, which could potentially be applied to other protocols other than the google RPC, so that it could potentially be extracted out into Jetty to be maintained in the future.

The interface with google code is done in the com.google.apphosting.runtime.jetty.delegate.impl package.

So I think adding compliance modes specific for the google usage should be here rather than in the DelegateConnector.

server.addConnector(rpcConnector);
}

AppVersionHandlerFactory appVersionHandlerFactory =
AppVersionHandlerFactory.newInstance(server, serverInfo);
appVersionHandler = new AppVersionHandler(appVersionHandlerFactory);
if (!Boolean.getBoolean(IGNORE_RESPONSE_SIZE_LIMIT)) {
CoreSizeLimitHandler sizeLimitHandler = new CoreSizeLimitHandler(-1, MAX_RESPONSE_SIZE);
sizeLimitHandler.setHandler(appVersionHandler);
server.setHandler(sizeLimitHandler);
} else {
server.setHandler(appVersionHandler);
server.setHandler(appVersionHandler);

// In HttpConnector mode we will combine both SizeLimitHandlers.
boolean ignoreResponseSizeLimit = Boolean.getBoolean(IGNORE_RESPONSE_SIZE_LIMIT);
if (!ignoreResponseSizeLimit && !isHttpConnectorMode) {
server.insertHandler(new SizeLimitHandler(-1, MAX_RESPONSE_SIZE));
}

boolean startJettyHttpProxy = false;
if (runtimeOptions.useJettyHttpProxy()) {
AppInfoFactory appInfoFactory;
Expand All @@ -159,14 +178,13 @@ public void run(Runnable runnable) {
} catch (Exception e) {
throw new IllegalStateException(e);
}
if (Boolean.getBoolean(HTTP_CONNECTOR_MODE)) {
if (isHttpConnectorMode) {
logger.atInfo().log("Using HTTP_CONNECTOR_MODE to bypass RPC");
JettyHttpProxy.insertHandlers(server);
server.insertHandler(
new JettyHttpHandler(
runtimeOptions, appVersionHandler.getAppVersion(), appVersionKey, appInfoFactory));
ServerConnector connector = JettyHttpProxy.newConnector(server, runtimeOptions);
server.addConnector(connector);
JettyHttpProxy.insertHandlers(server, ignoreResponseSizeLimit);
server.addConnector(JettyHttpProxy.newConnector(server, runtimeOptions));
} else {
server.setAttribute(
"com.google.apphosting.runtime.jetty.appYaml",
Expand Down Expand Up @@ -197,7 +215,7 @@ public void stop() {
}

@Override
public void addAppVersion(AppVersion appVersion) throws FileNotFoundException {
public void addAppVersion(AppVersion appVersion) {
appVersionHandler.addAppVersion(appVersion);
}

Expand Down Expand Up @@ -239,16 +257,7 @@ public void serviceRequest(UPRequest upRequest, MutableUpResponse upResponse) th
}
lastAppVersionKey = appVersionKey;
}
// TODO: lots of compliance modes to handle.
HttpConfiguration httpConfiguration = rpcConnector.getHttpConfiguration();
httpConfiguration.setSendDateHeader(false);
httpConfiguration.setSendServerVersion(false);
httpConfiguration.setSendXPoweredBy(false);
if (LEGACY_MODE) {
httpConfiguration.setRequestCookieCompliance(CookieCompliance.RFC2965);
httpConfiguration.setResponseCookieCompliance(CookieCompliance.RFC2965);
httpConfiguration.setUriCompliance(UriCompliance.LEGACY);
}

DelegateRpcExchange rpcExchange = new DelegateRpcExchange(upRequest, upResponse);
rpcExchange.setAttribute(AppEngineConstants.APP_VERSION_KEY_REQUEST_ATTR, appVersionKey);
rpcExchange.setAttribute(AppEngineConstants.ENVIRONMENT_ATTR, ApiProxy.getCurrentEnvironment());
Expand Down
21 changes: 14 additions & 7 deletions ..._impl_jetty12/src/main/java/com/google/apphosting/runtime/jetty/proxy/JettyHttpProxy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import com.google.apphosting.runtime.ServletEngineAdapter;
import com.google.apphosting.runtime.anyrpc.EvaluationRuntimeServerInterface;
import com.google.apphosting.runtime.jetty.AppInfoFactory;
import com.google.apphosting.runtime.jetty.CoreSizeLimitHandler;
import com.google.apphosting.runtime.jetty.JettyServletEngineAdapter;
import com.google.common.base.Ascii;
import com.google.common.base.Throwables;
Expand All @@ -44,9 +43,12 @@
import org.eclipse.jetty.server.Response;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.ServerConnector;
import org.eclipse.jetty.server.SizeLimitHandler;
import org.eclipse.jetty.server.handler.gzip.GzipHandler;
import org.eclipse.jetty.util.Callback;

import static com.google.apphosting.runtime.AppEngineConstants.IGNORE_RESPONSE_SIZE_LIMIT;

/**
* A Jetty web server handling HTTP requests on a given port and forwarding them via gRPC to the
* Java8 App Engine runtime implementation. The deployed application is assumed to be located in a
Expand All @@ -68,6 +70,7 @@
public class JettyHttpProxy {
private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
private static final long MAX_REQUEST_SIZE = 32 * 1024 * 1024;
private static final long MAX_RESPONSE_SIZE = 32 * 1024 * 1024;

/**
* Based on the adapter configuration, this will start a new Jetty server in charge of proxying
Expand Down Expand Up @@ -108,22 +111,26 @@ public static ServerConnector newConnector(
return connector;
}

public static void insertHandlers(Server server) {
CoreSizeLimitHandler sizeLimitHandler = new CoreSizeLimitHandler(MAX_REQUEST_SIZE, -1);
sizeLimitHandler.setHandler(server.getHandler());
public static void insertHandlers(Server server, boolean ignoreResponseSizeLimit) {

long responseLimit = -1;
if (!ignoreResponseSizeLimit) {
responseLimit = MAX_RESPONSE_SIZE;
}
SizeLimitHandler sizeLimitHandler = new SizeLimitHandler(MAX_REQUEST_SIZE, responseLimit);
server.insertHandler(sizeLimitHandler);

GzipHandler gzip = new GzipHandler();
gzip.setInflateBufferSize(8 * 1024);
gzip.setHandler(sizeLimitHandler);
gzip.setIncludedMethods(); // Include all methods for the GzipHandler.
server.setHandler(gzip);
server.insertHandler(gzip);
}

public static Server newServer(
ServletEngineAdapter.Config runtimeOptions, ForwardingHandler forwardingHandler) {
Server server = new Server();
server.setHandler(forwardingHandler);
insertHandlers(server);
insertHandlers(server, true);

ServerConnector connector = newConnector(server, runtimeOptions);
server.addConnector(connector);
Expand Down
Loading
Loading