vaadin / flow

Vaadin Flow is a Java framework binding Vaadin web components to Java. This is part of Vaadin 10+.
Apache License 2.0
622 stars 167 forks source link

VaadinServlet won't initialize under specific circumstances #20472

Open mvysny opened 1 week ago

mvysny commented 1 week ago

Description of the bug

Under certain circumstances (listed below), Vaadin isn't initialized properly and doesn't set the Lookup instance to the session. That causes VaadinServlet to return prematurely from the initialization, but via a return statement instead of throwing an exception (see VaadinServlet:126, the vaadinServletContext.getAttribute(Lookup.class) == null statement). That causes the servlet container to believe that VaadinServlet has initialized and is ready to serve the requests, but the Servlet is not ready: the staticFileHandler is null, which causes any request for static files to fail with a NullPointerException: Cannot invoke "com.vaadin.flow.server.StaticFileHandler.serveStaticResource(javax.servlet.http.HttpServletRequest, javax.servlet.http.HttpServletResponse)" because "this.staticFileHandler" is null.

Related tickets: #15991 and #10238

This happens when Vaadin app is starting in an embedded Jetty with classpath scanning disabled; Jetty won't discover and run LookupServletContainerInitializer which in turn won't create Lookup and won't set it to the servlet context.

More information at https://mvysny.github.io/npe-staticfilehandler-servestaticresources/

Expected behavior

Since VaadinServlet can not function properly without Lookup, VaadinServlet should throw an IllegalStateException or ServletException that "Vaadin wasn't initialized properly, Lookup is missing in ServletContext, probably LookupServletContainerInitializer wasn't called". Alternatively, a huge error message should be logged in the log, but the exception is far preferable over this.

Minimal reproducible example

https://github.com/vaadin/flow/issues/15991#issuecomment-1576056924 lists steps to reproduce the issue.

Versions

mcollovati commented 1 week ago

For the record, this is the change that introduced the check on Lookup presence: #9500.

If I get it correctly, it does not throw because on OSGi the Servlet init method can be called multiple times (https://github.com/vaadin/flow/pull/9500#discussion_r530816513)

mvysny commented 1 week ago

I suspected that the current bailout was in place because of OSGi. Perhaps, if the servlet can differentiate between an OSGi and non-OSGi environment, we can:

In either case, the return statement would benefit from additional documentation.

mshabarov commented 5 days ago

Throwing an exception look fine, it should suggest how to reconfigure Jetty to be compatible with Vaadin. By the way, what would be the recommendation? To enable the classpath scanning ?

mvysny commented 5 days ago

I think enabling the classpath scanning is the easiest way; and that's how servlet containers behave by default when you drop a WAR file in. Alternative would be to provide Jetty with a complete list of all @WebListeners, Servlets and other things; but that's a lot of work, the list can potentially change and therefore this approach is a bit fragile.

This is how Vaadin Boot enables classpath scanning in Jetty: https://github.com/mvysny/vaadin-boot/blob/1bac033680358f363ee8c9bcefe0e0ab175241e6/vaadin-boot/src/main/java/com/github/mvysny/vaadinboot/common/JettyWebServer.java#L156