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

Allow WebServers to be stopped and restarted #34955

Closed
tzolov opened this issue Apr 12, 2023 · 15 comments
Closed

Allow WebServers to be stopped and restarted #34955

tzolov opened this issue Apr 12, 2023 · 15 comments
Assignees
Labels
theme: checkpoint-restore Issues related to coordinated restore at checkpoint type: enhancement A general enhancement
Milestone

Comments

@tzolov
Copy link

tzolov commented Apr 12, 2023

On a plain web Spring Boot (3.0.5) application if one do context.stop() and then context.start() the context on a I get an error:

Exception in thread "main" org.springframework.context.ApplicationContextException: Failed to start bean 'webServerStartStop'
        at org.springframework.context.support.DefaultLifecycleProcessor.doStart(DefaultLifecycleProcessor.java:181)
        at org.springframework.context.support.DefaultLifecycleProcessor$LifecycleGroup.start(DefaultLifecycleProcessor.java:356)
        at java.base/java.lang.Iterable.forEach(Iterable.java:75)
        at org.springframework.context.support.DefaultLifecycleProcessor.startBeans(DefaultLifecycleProcessor.java:155)
        at org.springframework.context.support.DefaultLifecycleProcessor.start(DefaultLifecycleProcessor.java:103)
        at org.springframework.context.support.AbstractApplicationContext.start(AbstractApplicationContext.java:1418)
        at com.example.crac.springwebcrac.SpringWebCracApplication.main(SpringWebCracApplication.java:35)
Caused by: java.lang.IllegalStateException: The host does not contain a Context
        at org.springframework.boot.web.embedded.tomcat.TomcatWebServer.findContext(TomcatWebServer.java:153)
        at org.springframework.boot.web.embedded.tomcat.TomcatWebServer.start(TomcatWebServer.java:232)
        at org.springframework.boot.web.servlet.context.WebServerStartStopLifecycle.start(WebServerStartStopLifecycle.java:44)
        at org.springframework.context.support.DefaultLifecycleProcessor.doStart(DefaultLifecycleProcessor.java:178)

To reproduce create a plain web boot application SpringWebCracApplication like this:

	public static void main(String[] args) throws InterruptedException {
		ConfigurableApplicationContext context = SpringApplication.run(SpringWebCracApplication.class, args);
		context.stop();
		Thread.sleep(5000);
		context.start();
	}

It looks like the tomcat.destroy() , called on context.stop(), destroys the Tomcat.Context and later is not re-created on start.

Note that If we replace the Tomcat with a Jetty the application works fine and recovers after stop/start.

@jhoeller
Copy link

From a quick glance at the TomcatWebServer implementation, the this.tomcat.destroy() call might have to move to a DisposableBean.destroy() method so that Lifecycle.stop() only contains the stopTomcat() call - which should allow for a re-start() after stop() then. This is a necessity for snapshot restoring with Project CRaC but also applies to custom stop/start usage in application setups, according to the regular Spring lifecycle contract.

To be clear, in the Spring bean lifecycle, the main purpose of stop() is a pre-destruction signal propagated before the round of actual destroy() calls, allowing for some early shutdown signals to asynchronous workers etc. In such a scenario, the stop and destroy phases happen in immediate sequence, so too-early destruction steps in the stop phase won't be noticeable. However, conceptually, the stop call is meant to allow for a re-start as well (even if rarely used at this point). As a consequence, a stop() implementation needs to leave its bean instance in a state where destroy() could subsequently shut down for good, while that same post-stop state also needs to let start() pick it up from there again. That's the guideline that we have for separating stop/destroy logic at this point.

I recommend revising TomcatWebServer right away for Boot 3.1 since this is arguably a mismatch with the existing lifecycle contract and an inconsistency with its Jetty equivalent.

@jhoeller
Copy link

Since we might be using Spring Boot 3.1 for early demos with Project CRaC, it would be great to fix this ASAP for that reason as well. @wilkinsona @bclozel for your consideration, I hope this is a simple enough revision.

@wilkinsona
Copy link
Member

Depending on the complexity of the fix, I'd consider fixing this one in 3.0.x or even 2.7.x.

@wilkinsona wilkinsona added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 27, 2023
@wilkinsona wilkinsona changed the title The EmbeddedTomcat is not correctly resolved on context.start() IllegalStateException: The host does not contain a Context is thrown when TomcatWebServer is started after being stopped Apr 27, 2023
@wilkinsona
Copy link
Member

@markt-asf Looking at some differences in the state after calling stop() rather than destory() I've noticed that StandardServer leaves its utilityExecutor running after a call to stop(). Can you please help us to understand the implications of that? I'm wondering if it could result in something that Tomcat or application code has scheduled running when everything should have been stopped.

@wilkinsona wilkinsona added the status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team label Apr 27, 2023
@wilkinsona
Copy link
Member

There's a similar problem with Undertow. I've opened #35184.

@markt-asf
Copy link

Theoretically yes, but that would be a bug in Tomcat (or the application code) that should have shut those tasks down when the associated component shut down.
I did a quick scan of the Tomcat code and it looks like Tomcat does shut things down correctly.
We recently exposed the utility executor to applications via a ServletContext attribute but it isn't advertised and I doubt many applications are using it (and if they are they need to shut down tasks when the web app stops).

@wilkinsona
Copy link
Member

Thanks, Mark. While it sounds like we don't need to worry about the executor being left running, I wonder if it could be stopped in stop() (and then started/recreated again in start() if needed) to minimise the likelihood of a bug occurring?

@markt-asf
Copy link

That looks possible. I'll start a thread on the Tomcat dev@ list to discuss the possibility.

@wilkinsona wilkinsona removed the status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team label Apr 27, 2023
@wilkinsona
Copy link
Member

Separating stop() and destroy() isn't sufficient, unfortunately. Calling stop() resets the context such that it needs to be prepared again before it can be reused. At the moment, this preparation is done by the factory that creates the web server and not by the web server itself. To allow a restart after a stop, the web server itself will have to be able to perform that preparation.

@wilkinsona wilkinsona added this to the 3.1.x milestone May 2, 2023
@wilkinsona
Copy link
Member

Getting the context ready for use is currently split across three protected methods:

  • TomcatServletWebServerFactory.prepareContext(Host, ServletContextInitializer[])
  • TomcatServletWebServerFactory.configureContext(Context, ServletContextInitializer[])
  • TomcatServletWebServerFactory.postProcessContext(Context)

The reasons for the split aren't clear to me. prepareContext creates the context, configures several of its settings, adds it to the Host and then calls the other two methods. configureContext configures several more of the context's settings and also calls any TomcatContextCustomizers that are registered with the factory. postProcessContext is an empty method that can be overridden to post-process the context. As far as I can tell, the same could be achieved by overriding configureContext, calling super, and then configuring the context further.

@wilkinsona wilkinsona added the theme: checkpoint-restore Issues related to coordinated restore at checkpoint label May 16, 2023
@wilkinsona wilkinsona modified the milestones: 3.1.x, 3.x May 19, 2023
@somayaj

This comment was marked as outdated.

@wilkinsona

This comment was marked as outdated.

@somayaj

This comment was marked as outdated.

@philwebb

This comment was marked as outdated.

@wilkinsona wilkinsona changed the title IllegalStateException: The host does not contain a Context is thrown when TomcatWebServer is started after being stopped Allow Tomcat to be stopped and restarted Jun 26, 2023
@wilkinsona wilkinsona added type: enhancement A general enhancement and removed type: bug A general bug labels Jun 26, 2023
@wilkinsona wilkinsona self-assigned this Jun 26, 2023
@wilkinsona wilkinsona removed this from the 3.x milestone Jun 26, 2023
@wilkinsona wilkinsona added this to the 3.2.x milestone Jun 26, 2023
@wilkinsona wilkinsona changed the title Allow Tomcat to be stopped and restarted Allow WebServers to be stopped and restarted Jun 26, 2023
@wilkinsona
Copy link
Member

I've broadened the scope of this beyond Tomcat. We can also support stop and restart with Jetty (servlet and reactive). It does not appear to be possible to support stop and restart with Undertow, particularly for Servlet-based apps. It should also work with Reactor Netty, although perhaps not as efficiently as it could do. We can refine in the future as needs be.

@wilkinsona wilkinsona modified the milestones: 3.2.x, 3.2.0-M1 Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: checkpoint-restore Issues related to coordinated restore at checkpoint type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

7 participants