On Mon, 15 Nov 2021 12:58:45 GMT, Jaikiran Pai 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.
Thank you all for the inputs. Based on them, I'll close this PR.
-
PR: https://git.openjdk.java.net/jdk/pull/6389
