jetty / jetty.project

Eclipse Jetty® - Web Container & Clients - supports HTTP/2, HTTP/1.1, HTTP/1.0, websocket, servlets, and more
https://eclipse.dev/jetty
Other
3.85k stars 1.91k forks source link

Setting a StatisticsHandler makes http.spi.JettyHttpServer unstartable with exception #7752

Closed mattiasbe closed 1 year ago

mattiasbe commented 2 years ago

Jetty version(s) 9.4.45

Java version/vendor (use: java -version) 1.8.0_241; Java(TM) SE Runtime Environment 1.8.0_241-b07, Java HotSpot(TM) 64-Bit Server VM 25.241-b07; Oracle Corporation

OS type/version Windows 10 64bit

Description

Setting a StatisticsHandler makes http.spi.HttpServer unstartable with exception:

java.lang.RuntimeException: could not find ContextHandlerCollection, you must configure one
    at org.eclipse.jetty.http.spi.JettyHttpServer.createContext(JettyHttpServer.java:208)
    at org.eclipse.jetty.http.spi.JettyHttpServer.createContext(JettyHttpServer.java:272)

Reason: The method JettyHttpServer#findContextHandlerCollection(...) doesn't look in HandlerContainers, only HandlerCollections. StatisticsHandler is an instance of HandlerContainer.

Adding the following (with the same pattern as the previous if statement there) to the for loop of that method solves the problem:

if (handler instanceof HandlerContainer) {
  HandlerContainer hc = (HandlerContainer) handler;
  ContextHandlerCollection chc = hc.getChildHandlerByClass(ContextHandlerCollection.class);
  if (chc != null)
      return chc;
}

diff:

$ diff -u JettyHttpServer_9.4.45.java JettyHttpServer.java
--- JettyHttpServer_9.4.45.java 2022-03-18 17:07:47.607197000 +0100
+++ JettyHttpServer.java        2022-03-18 17:05:52.812724100 +0100
@@ -29,6 +29,7 @@
 import com.sun.net.httpserver.HttpHandler;
 import org.eclipse.jetty.server.Connector;
 import org.eclipse.jetty.server.Handler;
+import org.eclipse.jetty.server.HandlerContainer;
 import org.eclipse.jetty.server.HttpConfiguration;
 import org.eclipse.jetty.server.HttpConnectionFactory;
 import org.eclipse.jetty.server.NetworkConnector;
@@ -278,6 +279,13 @@
                 if (chc != null)
                     return chc;
             }
+
+            if (handler instanceof HandlerContainer) {
+                HandlerContainer hc = (HandlerContainer) handler;
+                ContextHandlerCollection chc = hc.getChildHandlerByClass(ContextHandlerCollection.class);
+                if (chc != null)
+                    return chc;
+            }
         }
         return null;
     }

How to reproduce?

If having having done these two things:

  1. set JettyHttpServerProvider for example by:
    System.setProperty("com.sun.net.httpserver.HttpServerProvider", JettyHttpServerProvider.class.getName());
    JettyHttpServerProvider.setServer(server)
  2. Adding a statistics handler:
    StatisticsHandler stats = new StatisticsHandler();
    stats.setHandler(handlers);
    server.setHandler(stats);

=> Upon Endpoint.publish(...):

java.lang.RuntimeException: could not find ContextHandlerCollection, you must configure one
    at org.eclipse.jetty.http.spi.JettyHttpServer.createContext(JettyHttpServer.java:208)
    at org.eclipse.jetty.http.spi.JettyHttpServer.createContext(JettyHttpServer.java:272)
...
joakime commented 2 years ago

Set server.setDumpAfterStart(true); when you create your server instance, but before you start it, and report back the output.

In step 2, you don't describe what handlers is.

Also, your version of Java (1.8.0_241) expired on May 14, 2020 - you need to upgrade it.

mattiasbe commented 2 years ago

Thanks for the really quick reply.

Set server.setDumpAfterStart(true); when you create your server instance, but before you start it, and report back the output.

I'll see if I can minimize it somewhat or make a minimum standalone reproducable case instead.

In step 2, you don't describe what handlers is. In this case:

ContextHandlerCollection contexts = new ContextHandlerCollection();
HandlerCollection handlers = new HandlerCollection();
handlers.setHandlers(new Handler[] { contexts, new DefaultHandler() });

But multiple things adding/using the contexts... I'll update later on.

Also, your version of Java (1.8.0_241) expired on May 14, 2020 - you need to upgrade it.

Thanks for pointing this out, it's just one java 8 jdk folder I had on my workstation and I could reproduce it with the first one I found so went with that.

mattiasbe commented 2 years ago

Attached the server dump, might be a bit much in the classloader when I quickly reproduced it, but relevant information seems to be there.

serverdump.txt .

mattiasbe commented 2 years ago

That was with the patched. Here is without the patch. Along with the complete exception and stacktrace afterwards.

serverdump_unpatched.txt .

mattiasbe commented 2 years ago

A minimal reproducible case is attached along with the server dump.

serverdump_unpatched.txt StatisticsJettyProblem.java.txt .

mattiasbe commented 2 years ago

Anything else needed?

joakime commented 2 years ago

Note: Jetty 9.4.x is now at End of Community Support. (See issue for details)


From StatisticsJettyProblem.java.txt

+= StatisticsHandler@5622fdf{STARTED,r=0,d=0} - STARTED
|  += HandlerCollection@4883b407{STARTED} - STARTED
|     += ContextHandlerCollection@7d9d1a19{STARTED} - STARTED
|     += DefaultHandler@39c0f4a{STARTED} - STARTED
+= ErrorHandler@1794d431{STARTED} - STARTED
...(snip)...
key: +- bean, += managed, +~ unmanaged, +? auto, +: iterable, +] array, +@ map, +> undefined
2022-03-18 21:14:43.107:INFO:oejs.Server:main: Started @726ms
Exception in thread "main" com.sun.xml.internal.ws.server.ServerRtException: Server Runtime Error: java.lang.RuntimeException: could not find ContextHandlerCollection, you must configure one
    at com.sun.xml.internal.ws.transport.http.server.ServerMgr.createContext(ServerMgr.java:130)
    at com.sun.xml.internal.ws.transport.http.server.HttpEndpoint.publish(HttpEndpoint.java:64)
    at com.sun.xml.internal.ws.transport.http.server.EndpointImpl.publish(EndpointImpl.java:232)
    at com.sun.xml.internal.ws.spi.ProviderImpl.createAndPublishEndpoint(ProviderImpl.java:126)
    at javax.xml.ws.Endpoint.publish(Endpoint.java:240)
    at scratch.StatisticsJettyProblem.main(StatisticsJettyProblem.java:47)
Caused by: java.lang.RuntimeException: could not find ContextHandlerCollection, you must configure one
    at org.eclipse.jetty.http.spi.JettyHttpServer.createContext(JettyHttpServer.java:208)
    at org.eclipse.jetty.http.spi.JettyHttpServer.createContext(JettyHttpServer.java:273)
    at com.sun.xml.internal.ws.transport.http.server.ServerMgr.createContext(ServerMgr.java:102)

Congrats you have a StatisticsHandler in your handler tree. Thing is, you don't have the rest of the handler tree setup properly. Try using createHttpServer() and then just modifying the server handler tree by wrapping the default handlers that http-spi needs.

This works with Jetty 10.0.x btw.

HttpServer server = new JettyHttpServerProvider().createHttpServer(new
    InetSocketAddress("localhost", 0), 10));

JettyHttpServer jettyServer = (JettyHttpServer) server;
jettyServer.getServer().setDumpAfterStart(true);
Handler originalRootHandler = jettyServer.getServer().getHandler();
StatisticsHandler rootHandler = new StatisticsHandler();
rootHandler.setHandler(originalRootHandler);
jettyServer.getServer().setHandler(rootHandler);

final HttpContext httpContext = server.createContext( // ... etc ...
mattiasbe commented 2 years ago

Hi thanks for the quick reply. Yes I'm aware of the end of community support since May but hoped that as this was reported back in March that the suggested patch in the original post would end up in a release before that. :)

It's a small patch and non-intrusive IMO and just like the if statement before that.

joakime commented 2 years ago

Due to legal reasons we cannot accept patches as issues or issue comments.

Only via a formal PR with signed ECA. This is a requirement by Eclipse Legal that we have zero wiggle room with.

mattiasbe commented 2 years ago

Understood (for the record I've previously signed ECA 3.0.0 and updated to the latest one now 3.1.0) but would you agree that what's stated in the original post under reason:

The method JettyHttpServer#findContextHandlerCollection(...) doesn't look in HandlerContainers, only HandlerCollections. StatisticsHandler is an instance of HandlerContainer.

...is missing or at least of value if it wasn't left out? I appreciate you keep answering here but I don't want to waste anyone's valuable time.

Would it have been better if no source code investigation was made and mentioning what seems to be missing (honest question)? If not done as a proper PR. Does this make it difficult for a commiter to do this for example?

Would a proper ECA signed PR now even be of use?

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale because it has been a full year without activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions[bot] commented 1 year ago

This issue has been closed due to it having no activity.