-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 better configuration of WebAppContext classloader #10163
Allow better configuration of WebAppContext classloader #10163
Conversation
Moved the creation of a WebAppContext classloader into an extensible method.
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.
We need this for EE8/EE9.
jetty-ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/WebAppContext.java
Show resolved
Hide resolved
…bapp-classloader-configuration
if (loader instanceof URLClassLoader) | ||
((URLClassLoader)loader).close(); | ||
if (loader instanceof URLClassLoader urlClassLoader) | ||
urlClassLoader.close(); |
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.
Wouldn't this break a restart of the context?
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.
That's a good question... But existing behavior
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.
@lachlan-roberts I cleaned this up. Can you re-review?
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.
Not sure I understand the purpose of this... Can you explain the logic behind this change?
loader
will always be a WebAppClassLoader
so this will never be true, unless there is an overridden configureClassLoader
configured to return a URLClassLoader
. If this is the case can't they also override stopContext
and close it themselves?
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.
@lachlan-roberts WebAppClassLoader
is-a URLClassLoader
, so this will always be true when the current loader is not the initial loader (i.e. we created the loader during doStart). So probably could assume that, but instanceof check is nice way to do the caste without a warning.
Only close loader if context created it.
Punting this to 12.0.1 |
Moved the creation of a WebAppContext classloader into an extensible method.