APEXCORE-767.set parent classloader in StramAppLauncher.loadDependencies #563
Conversation
|
Test this please |
|
@florianschmidt1994 please change your git settings so that the email address matches your github profile, otherwise the commits won't be linked. |
|
Please include JIRA info into commit message as well. |
| URLClassLoader cl = URLClassLoader.newInstance(launchDependencies.toArray(new URL[launchDependencies.size()])); | ||
| URL[] dependencies = launchDependencies.toArray(new URL[launchDependencies.size()]); | ||
|
|
||
| ClassLoader currentContextClassLoader = Thread.currentThread().getContextClassLoader(); |
There was a problem hiding this comment.
Can created URL class loaders be cached, it should not be necessary to create a new class loader each time loadDependencies() is called.
There was a problem hiding this comment.
Possibly. The dependencies can change though, between original introspection of the app package and actual launch, for example when the application adds additional jar files to LIBRARY_JARS. And as commented on the JIRA, class loaders also need to be cleaned up..
There was a problem hiding this comment.
The suggestion is to cache URL class loaders on the instance of the StramAppLauncher instance to avoid unnecessary instantiation of URL class loaders with the same dependencies. If dependencies change, the cache should be cleared.
There was a problem hiding this comment.
The issue to be addressed here it that additional dependencies need to be added to the existing class loader, hence chaining with previous class loader. Avoiding repeated instantiation with exactly same dependencies is an optimization, first we should get the functional issue addressed.
There was a problem hiding this comment.
While it will be OK just to fix the functional issue in this PR and have JIRA open with optimization request it will be great if the contributor is willing to put a little bit more effort and implement the optimization. Said that I will not stop any other committer from merging the PR if it looks sufficient.
There was a problem hiding this comment.
I'm not suggesting to merge the PR just yet, I wan't to make sure we actually resolve the issue before applying optimizations. For example, the PR needs to address removing the prior context class loader before the next application is launched.
|
@florianschmidt1994 Please consider adding a unit test. |
florianschmidt1994
force-pushed
the
APEXCORE-767.duplicate-classloading-in-cli-local
branch
from
4ce4858
to
759e5c3
Compare
florianschmidt1994
force-pushed
the
APEXCORE-767.duplicate-classloading-in-cli-local
branch
4 times, most recently
from
9316a89
to
3fa4b14
Compare
|
@florianschmidt1994 please rebase the PR and see if the CI build passes. Also change the author on the commit to reflect your github account: http://apex.apache.org/contributing.html#github-and-git |
| @@ -0,0 +1,61 @@ | |||
| /** | |||
There was a problem hiding this comment.
Replace with the standard license header.
There was a problem hiding this comment.
Can I replace this with our license header when I de facto just copy and pasted this code from stack overflow?
There was a problem hiding this comment.
Is this needed at all, can you not use ExecutorService instead? https://stackoverflow.com/a/40605786/3148138
Regarding license header, all files have to have the ASL header. As to whether you can copy the code or not:
You don’t have to include the full MIT License in your code base. Contributors agree to give code users permission to ignore the MIT License’s notice preservation requirement, as long as users give reasonable attribution upon request of the copyright holder (or Stack Exchange on behalf of the contributor). This optional exception to the MIT License will live in our terms of service.
|
I have verified that this fixes APEXMALHAR-2511. Please rebase, fix license header and commit author. |
|
@florianschmidt1994 any update? |
|
There are still some minor details that I wanted to verify, I'll take care of this during the course of the week |
florianschmidt1994
force-pushed
the
APEXCORE-767.duplicate-classloading-in-cli-local
branch
from
3fa4b14
to
de01bef
Compare
I added this table from JIRA with cases where potentiall calls to resetContextClassLoader might be necessary. Unfortunately some of the details of whats happening in the cases marked with |
|
YarnLauncherImpl#launchApp - before the return |
|
@florianschmidt1994 any update? |
StramAppLauncher.loadDependencies is called multiple times when starting an application via the apex-cli with the -local option. In each of the calls to loadDependencies, the contextClassLoader of the current thread would be replaced with a new instance of URLClassLoader (which has no parent class loader set). This can lead to issues, e.g. when one aquires the current contextClassLauncher, loads a class with it and tries to cast it to a class which was loaded with a previous version of the contextClassLoader, resulting in a ClassCastException. An example of this bug can be seen in APEXMALHAR-2511 The changes in this commit fix this by passing the parent class loader for each new instance of URLClassLoader to the current contextClassLoader
florianschmidt1994
force-pushed
the
APEXCORE-767.duplicate-classloading-in-cli-local
branch
from
de01bef
to
34d71f4
Compare
|
The changes were incorporated as discussed and it should be looking good by now. Unfortunately I don't have a setup up and running to test this right now, but if wanted I could sit down and verify the changes one last time in the near future. Otherwise feel free to merge (if the build passes ;) ) |
|
FAILURE --none-- |
|
test this please |
|
FAILURE --none-- |
|
test this please |
|
SUCCESS --none-- |
florianschmidt1994
deleted the
APEXCORE-767.duplicate-classloading-in-cli-local
branch
Following below is my understanding of the problem (should be probably taken with a grain of salt, as I just started digging in to the apex-core codebase):
StramAppLauncher.loadDependencies is called multiple times when starting
an application via the apex-cli with the -local option. In each of the
calls to loadDependencies, the contextClassLoader of the current thread
would be replaced with a new instance of URLClassLoader (which has no
parent class loader set).
This can lead to issues, e.g. when one acquires the current
contextClassLauncher, loads a class with it and tries to cast it to a
class which was loaded with a previous instance of the contextClassLoader,
resulting in a ClassCastException.
An example of this bug can be seen in APEXMALHAR-2511
The changes in this pr fix this by passing the parent class loader
for each new instance of URLClassLoader to the current
contextClassLoader
I am still in the process of figuring how the StramAppLauncher works and why the dependencies get loaded multiple times in the first place, is this intended behavior? Otherwise I could also look into what is going on there as well. (See discussion on JIRA)