-
Notifications
You must be signed in to change notification settings - Fork 307
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
FISH-9171 bugfix: prevent deployment failures from causing an undeployment failure and leaks #6848
Conversation
3d791b2
to
f3f7abf
Compare
…ure, and leaking ClassLoader resources in the process
f3f7abf
to
f899f65
Compare
@Pandrex247 This one's ready to go :) |
This won't make the imminent release as we've just frozen for it, but I Should™ get round to reviewing it for the next one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any kind of reproducer for the scenario you're attempting to address here?
I can easily see that the LifecycleException
being thrown will cause it to essentially skip all remaining unloading, but the force loading of JSP during exception handling and only logging undeployment errors if there's no deployment context seem a bit odd to me and I'd like to debug it.
appserver/web/web-core/src/main/java/org/apache/catalina/core/StandardContext.java
Outdated
Show resolved
Hide resolved
@@ -2058,7 +2061,7 @@ public void unloadWebModule(String contextRoot, String appName, String virtualSe | |||
} | |||
} | |||
|
|||
if (!hasBeenUndeployed) { | |||
if (!hasBeenUndeployed && deployment.getCurrentDeploymentContext() == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what this meant to be addressing.
&&
operator means you're restricting this log message down to an even more specific case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, this message is printed during deployment, and that's an error.
The message is intended to be printed during undeployment only.
This change will check if the deployment process is running, and if it is, it will inhibit this message.
The result is that the message will only be printed during "true" undeployment as it was intended to be originally.
Pretty much any application that fails deployment will be a valid reproducer for this Here is the specific one I was testing with: https://github.com/flowlogix/shiro-env-vars |
…StandardContext.java Co-authored-by: Andrew Pielage <pandrex247@hotmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
FISH-9171 bugfix: prevent deployment failures from causing an undeployment failure and leaks
Description
Bugfix. Application Deployment failure results Class Loader leak and extra error messages.
The root cause is the message stops undeployment process from proceeding further.
This is now fixed and leaks are removed.
Important Info
Testing
Testing Performed
Using ClassloaderDataAPI to test for leaks