On Mon, 15 Nov 2021 12:58:45 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
> Can I please get a review for this change which proposes to fix the issue > noted in https://bugs.openjdk.java.net/browse/JDK-8227020? > > The linked issue talks about non-normalized paths contributing to the leak. > However, the root cause is applicable for regular normalized paths too. The > `URLClassLoader` is backed by an instance of > `jdk.internal.loader.URLClassPath`. This `URLClassPath` keeps track of a > `loader` for each of the paths that are either provided directly as the > `URL[]` or are inferred through the `Class-Path` attribute of each loaded > jar. This it does in 2 different fields. One is a mapping between the String > representation of the URL for which the loader was created. This field is the > `lmap` and the other is a list of `loaders`. The list of `loaders` is the > search path for loading any resources from this `URLClassPath`. When the > `closeLoaders()` is called by the `URLClassLoader`, this `loaders` list is > iterated to close each of the `loader` for the opened resources (which in the > context of this issue translates to closing a `java.util.jar.JarFile`). > > A jar can have a `META-INF/INDEX.LIST`, which is a index of resources listed > against each jar file. The `URLClassPath` when looking for resources in a > `Loader` parses this index to search for resources. While doing so, if it > finds a jar listed in the index, for which a `Loader` hasn't yet been created > then it creates a new loader for such a jar and adds it to the `lmap`. > However, it doesn't add this to the `loaders` list, the one which is used to > close the opened jar files. As a result, it ends up opening a `JarFile` which > is never closed, even if the `URLClassPath`'s `closeLoaders()` is called, > thus triggering a leak. > > The commit in this PR adds the newly created loader to the list of `loaders` > so that it is properly closed, when the `closeLoaders` gets called. > > I have reviewed the code to see if this has any other side effects and from > what I can see, the addition of this new loader to the list shouldn't cause > any other issues for reasons that follow: > > - the order in the `loaders` list doesn't seem to have any one-to-one mapping > with the URL[] of classpath elements. In fact, loaders for newly discovered > URLs from the `Class-Path` attribute get added dynamically to this list. So > adding this new loader found in the index shouldn't cause any issues when it > comes to the order of loaders in the list. > - The `URLClassPath` is already returning resources from a loader > corresponding to a jar `X` listed in the index even if that jar `X` isn't > part of the original URL[] forming the classpath of the `URLClassPath`. So > adding this `loader` to the "search path" won't introduce any new behaviour. > > A new jtreg test has been introduced to reproduce this issue and verify the > fix. The test has been run on a Windows setup where the test reproduces this > issue without this fix and verifies the fix. > > Recently, the jar index feature has been disabled as part of > https://bugs.openjdk.java.net/browse/JDK-8273401. However, since users can > enable this feature by using a specific system property and since this issue > relates to a leak, I decided to see if there is interest in this fix. (Daniel, here's an issue https://bugs.openjdk.java.net/browse/JDK-8224794 reading: _"ZipFile/JarFile should open zip/JAR file with FILE_SHARE_DELETE sharing mode on Windows"_.) ------------- PR: https://git.openjdk.java.net/jdk/pull/6389
