jdaugherty commented on code in PR #16741:
URL: https://github.com/apache/grails-core/pull/16741#discussion_r4361983943
##########
grails-undertow/spring-boot/src/main/java/org/apache/grails/undertow/core/servlet/UndertowServletWebServerFactory.java:
##########
@@ -219,12 +222,13 @@ public void setPreservePathOnForward(boolean
preservePathOnForward) {
@Override
public WebServer getWebServer(ServletContextInitializer... initializers) {
Builder builder = createBuilder(this, this::getSslBundle,
this::getServerNameSslBundles);
- DeploymentManager manager = createManager(initializers);
+ DeploymentManager manager = createManager(new TempDirs(getPort()),
initializers);
return getUndertowWebServer(builder, manager, getPort());
}
- private DeploymentManager createManager(ServletContextInitializer...
initializers) {
+ private DeploymentManager createManager(TempDirs tempDirs,
ServletContextInitializer... initializers) {
DeploymentInfo deployment = Servlets.deployment();
+ deployment.addDeploymentCompleteListener(new
CleanTempDirsListener(tempDirs));
Review Comment:
Undertow runs the deployment-complete listeners' `contextDestroyed` at the
top of `DeploymentManagerImpl.undeploy()`, before `deployment.destroy()` calls
the application's own `ServletContextListener`s, so the directory the servlet
context exposes as `jakarta.servlet.context.tempdir` is already gone while
application listeners are still in `contextDestroyed`. Tomcat's
`CleanTempDirsListener` cleans up on the server's `AFTER_DESTROY_EVENT` and
Jetty's on `lifeCycleStopped` of the `Server`, both after the context has been
destroyed.
I probed it with a listener registered through a `ServletContextInitializer`
that records `((File)
servletContext.getAttribute(ServletContext.TEMPDIR)).isDirectory()` in
`contextDestroyed`: on this branch it records `false` after `stop()`.
Registering the cleanup as an ordinary listener, first in the
`DeploymentInfo`, keeps Tomcat's and Jetty's order:
`ApplicationListeners.contextDestroyed()` walks the listeners in reverse
registration order, and the listeners the initializers add come after the
`DeploymentInfo` ones, so a listener added here runs last:
```java
deployment.addListener(new ListenerInfo(CleanTempDirsListener.class,
new ImmediateInstanceFactory<>(new CleanTempDirsListener(tempDirs))));
```
With that the probe records `true` and the module's tests, including the
three new ones, pass. Could you switch to that and keep the probe as a test?
##########
grails-undertow/spring-boot/src/main/java/org/apache/grails/undertow/core/servlet/UndertowServletWebServer.java:
##########
@@ -92,4 +92,17 @@ public DeploymentManager getDeploymentManager() {
return this.manager;
}
+ /**
+ * Stops the server, which undeploys the deployment of a server that
started, and
+ * undeploys the deployment of a server that never started, so that the
listeners of
+ * its servlet context are destroyed and its temporary directories deleted.
+ */
+ @Override
+ public void destroy() {
+ super.destroy();
+ if (this.manager != null && this.manager.getState() ==
DeploymentManager.State.DEPLOYED) {
Review Comment:
A deployment is also left behind when the handler chain fails to build after
the deployment started. `DeploymentManagerHandler` starts the manager in its
constructor, so when a later factory throws from `createHttpHandler()`, for
instance `AccessLogHttpHandlerFactory` when the access log directory cannot be
created, `undertow` is still null, `destroySilently()` closes nothing, and the
manager stays `STARTED`. Boot's `ServletWebServerApplicationContext.refresh()`
then calls `stop()` and `destroy()` on the failed server, and this guard skips
a `STARTED` deployment, so `contextDestroyed` never runs and the directories
stay.
I probed it with `setAccessLogEnabled(true)` and an access log directory
under a regular file: `start()` throws `WebServerException`, and after `stop()`
and `destroy()` a registered listener's `contextDestroyed` has not run and the
`undertow.<port>.*` directory still exists.
Stopping a started deployment and undeploying whatever is not `UNDEPLOYED`
covers both cases, and the module's tests pass with it:
```java
try {
if (this.manager.getState() == DeploymentManager.State.STARTED) {
this.manager.stop();
}
} catch (ServletException ex) {
throw new WebServerException("Unable to stop Undertow deployment", ex);
}
if (this.manager.getState() != DeploymentManager.State.UNDEPLOYED) {
this.manager.undeploy();
}
```
Worth a test next to `aServerThatNeverStartedIsUndeployedWhenItIsDestroyed`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
