Community
Participate
Working Groups
PackageAdmin specification says that it will refresh all the bundles that are either updated or depending on updated bundles. It is enough to be included in the graph build by PackageAdmin for a bundle to be refreshed. This must cause assignement of a new classloader to a bundle. If such a bundle (not updated directly, but refreshed because one of the bundles it relied on was updated) contains a native library it will result in JVM error stating that the library was loaded by another classloader. Possible solution would be to copy or move bundle files to another directory on refresh so the library could be loaded again by another classloader (as it is identified by Java by it's disk location)
I do not really like copying the complete content of the bundle each time a refresh is done even if the bundle contains no native code. Seems like a better approach may be to keep track of when a native library was loaded. If an attempt to load it again occurs then copy it to a temp file and return that path.
A "copy-on-write" policy would be better (as Tom suggests). Everytime a bundle is refreshed, its native libs should be marked dirty. When then next findLibrary call is made, the dirty bit will tell the framework to make a copy to a new absolute path and return the new absolute path.
Sure. I'm not saying that it would be needed to be done each time. It could be quite easy to check whether bundle contains any native libraries. Of course "lazy approach" is good as well. The temp file would have other name and would be located in the same dir? Any thoughts on it? Would it be located in temp dir?
The fix for this should be isolated to BaseData.findLibrary method. Before returning the result we could add the native path string to a cache (Set). Each time findLibrary is called we would check to see if the result is in the cache. If it is then we know this library was loaded before, at this point we should copy the library located at the path to a temp file. We could have a templibs/ directory under the org.eclipse.osgi configuration area were we place the temp libs. We cannot just use the parent directory where the original library is located because this could be a directory under an extracted "directory" bundle which was installed by reference. In this case we do not want to add files to the plugin's directory.
Sounds good. However we still need a way of avoiding potential naming conflicts in the templibs directory. File.createTempFile can be used but this would change the name of library itself. Other solution would be to create random directory in templibs and copy there. Then library name would be preserved. We also need a way of cleaning this directory, most likely on a startup - I guess we don't want to have disk space leak here. What would be the best place for the cleaning code?
Created attachment 50101 [details] patch Here is a patch to demostrate what I'm thinking. - Each ClassPathManager (e.g. class loader) keeps track of the native code paths it finds. If the same library name is asked multiple times then it returns the previous path. This is required because you can call System.loadLibrary multiple times and it will only load it once for the calling classloader. In this case we do not want to copy on findLibrary. - Each BundleData also keeps track of when findLibrary is called multiple times for the same library. If it is called more than once then we copy the found library to a tempdir and return the temporary path. The double house keeping is needed so that we only copy library when a more than one classloader asks for the same library from the same BundleData object. The patch needs some work. For some reason the libraries are not being cleaned up on exit even though I call File.deleteOnExit() for the temp file. I think this is because the library file is still locked by the VM when it loaded the library. We will need to do a compact operation on the templib dir on startup. This should be quick if the templib dir does not exist. We also have some code that will set execution permissions on native code if a property is set. We should also do the same thing for the libraries we copy. I will finish off the patch if we agree this is the proper approach. This type of solution would also help if we ever decide to allow fragments to attach to more than one hosts. In this case the same fragment could deliver native code to more than one host bundle.
Fix looks good. Is there any chance that this fix will make 3.2.x release? Our next release will be 3.2 based and we need this kind of functionality quite urgently.
Created attachment 50562 [details] patch Here is a updated patch that addresses the remaining issues in comment 6, and adds some much needed comments to explain what we are doing. I will release this patch for 3.3 M3. We have missed the release for 3.2.1. The patch will need some extra testing and review to before we can allow it to be released into 3.2.x. Either way it can not make it until 3.2.2 (I do not know the dates for that, but I would not expect it until 1Q 2007)
Jeff's approval will be needed to release this to 3.2.2.
released latest patch to HEAD for 3.3 M3.
Need some info on risk and reqiurements before committing to 3.2.2
Created attachment 55163 [details] 3.2.x patch This is a patch against 3.2.x I put this patch risk at medium. It is not just a small code change. The code has been in Eclipse 3.3 since before M3, but typical eclipse scenarios do not run into this situation. We have not seen any "bad" effects of the patch in 3.3. This code should only take effect if a bundle classloader is refreshed and it contains native code (something that currently fails in the current 3.2.1 release). Jeff do you approave this fix for 3.2.2? Grzegorz is this fix still important to you? Can you wait til 3.3?
+1 if there are compelling usecases/needs to support the medium risk.
Thomas, Jeff, this fix is quite important for us as we will be basing our next release on LWI 7.1 containing Eclipse 3.2.x. So yes - we still do need it.
Fix released for 3.2.2.
