WebServer is not destroyed when ReactiveWebServerApplicationContext refresh fails#44134
WebServer is not destroyed when ReactiveWebServerApplicationContext refresh fails#44134nosan wants to merge 1 commit intospring-projects:3.3.xfrom
Conversation
| WebServer webServer = getWebServer(); | ||
| if (webServer != null) { | ||
| webServer.stop(); | ||
| webServer.destroy(); |
There was a problem hiding this comment.
Doesn't it make sense to destroy webServer even if stop() throws an exception?
Probably out of the scope for this fix, but it looks like Juergen was thinking about calling destroy() method in context of a bean lifecycle: #34955 (comment)
Thank you for the quick action in regard #44101! 🙏
There was a problem hiding this comment.
I don’t think so. If stop() throws an exception, it likely means that some resources, such as files, networks, etc. are still in use, which could cause destroy() to fail as well.
|
An alternative approach is 3.3.x...nosan:spring-boot:44101-alternative, which always tries to stop and destroy |
| } | ||
| } | ||
| catch (Exception shutdownEx) { | ||
| ex.addSuppressed(shutdownEx); |
There was a problem hiding this comment.
Let's consider this separately please. If we do it, it should be applied on the servlet side as well.
There was a problem hiding this comment.
Sure, I've reverted these changes.
There was a problem hiding this comment.
We think that adding the stop or destroy failure as a suppressed exception is a good idea. Would you like to open a PR for it or would you prefer that we take care of it?
Prior to this commit, if ReactiveWebServerApplicationContext failed to refresh, only WebServer.stop() was called. This commit additionally invokes WebServer.destroy(), aligning the behavior with ServletWebServerApplicationContext when a refresh failure occurs. Signed-off-by: Dmytro Nosan <dimanosan@gmail.com>
Prior to this commit, if ReactiveWebServerApplicationContext failed to refresh, only WebServer.stop() was called. This commit additionally invokes WebServer.destroy(), aligning the behavior with ServletWebServerApplicationContext when a refresh failure occurs. See gh-44134 Signed-off-by: Dmytro Nosan <dimanosan@gmail.com>
Prior to this commit, if
ReactiveWebServerApplicationContextfailed to refresh, onlyWebServer.stop()was called.This commit additionally invokes
WebServer.destroy(), aligning the behaviour withServletWebServerApplicationContextwhen a refresh failure occurs.gh-44101
I am also thinking about these changes as well:
3.3.x...nosan:spring-boot:44100-1