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.

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

fix: Disable Jetty's webapp configuration #6050

Merged
merged 2 commits into from
Sep 18, 2024

Conversation

niloc132
Copy link
Member

Simplifies embedded server setup slightly, and avoid calling reflective and ServiceLoader based configuration apis.

Fixes #6049

Simplifies embedded server setup slightly, and avoid calling reflective
and ServiceLoader based configuration apis.

Fixes deephaven#6049
String knownFile = "/ide/index.html";
URL ide = JettyBackedGrpcServer.class.getResource(knownFile);
Resource jarContents = Resource.newResource(ide.toExternalForm().replace("!" + knownFile, "!/"));
context.setBaseResource(ControlledCacheResource.wrap(jarContents));
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we don't use io.deephaven.server.jetty.ControlledCacheResource; is that correct? If so, should we delete it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, same problem that js-plugins has... I found that the ServletContextHandler also can be given a base resource and will guide DefaultServlet to provide content from there. We still want to share other settings though - it turns out our DefaultServlet configuration was coming from a jetty xml file, webdefault.xml. Now we explicitly configure it (and don't end up with other annotation scanning from the classpath or service loaders).

JamesXNelson
JamesXNelson previously approved these changes Sep 11, 2024
new ServletContextHandler(null, "/", null, null, null, new ErrorPageErrorHandler(), NO_SESSIONS);
String knownFile = "/ide/index.html";
URL ide = JettyBackedGrpcServer.class.getResource(knownFile);
String path = ide.toExternalForm().replace("!" + knownFile, "!/");
Copy link
Member

Choose a reason for hiding this comment

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

probably worth a comment about ""!/" specifies this resource base is a jar file which should have its contents served" (but worded better maybe)

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted to roughly the old code, and added a note.

@niloc132 niloc132 merged commit ca4ccb9 into deephaven:main Sep 18, 2024
16 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review Jetty startup, disable service loaders
3 participants