RFR: 8247407: tools/jlink/plugins/CompressorPluginTest.java test failing [v2]

[Jim Laskey]

On Sat, 25 Jun 2022 02:45:54 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Can I please get a review for this change which fixes an issue in `jdk.internal.jimage.ImageReader`? 
>> 
>> The `ImageReader` maintains a map of `nodes` which it uses to keep track of directories/resources from within the image. When a `findNode` is called, it leads to building the `Node` if the node isn't present in the map or if isn't complete (i.e. if the directory's immediate child nodes haven't been built). During this building of nodes, the code can end up creating a directory node and then visiting the child resources to create nodes for those resources. While doing so, the code currently doesn't check if a (child) node for the resource was already created previously (during a different call) and added to the parent directory's `children` list. As a result, it ends up adding the child node more than once.
>> 
>> One way to solve this would have been to make the `children` in `Directory` be a `Set`. I suspect it's currently a `List` so as to keep track of the order? If we did make it a `Set`, we could still keep the order using a `LinkedHashSet`. But changing this to a `Set` doesn't look feasible because there's a `List<Node> getChildren()` API on `Directory`, which would then have to create a copy of the `Set` to return as a `List`.
>> 
>> The commit in this PR instead prevents adding the child more than once by checking the `nodes` map for the presence of the child. This new check is added only for "resources" and not directories, because `makeDirectory` already skips a `Node` creation if the `nodes` has the corresponding entry (line 564 in the current PR).
>> Additionally an `assert` has been added to `addChild` of `Directory`.
>> 
>> A new jtreg test has been added which reproduces the issue without this change and verifies the fix.
>> 
>> This part of the code is very new to me, so if there's anything more to be considered, then please let me know.
>> 
>> tier1, tier2, tier3 testing is currently in progress.
>
> Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - Merge latest from jdk19 master
>  - 8288744: Remove tools/jlink/plugins/CompressorPluginTest.java from problemlist
>  - 8247407: tools/jlink/plugins/CompressorPluginTest.java test failing

LGTM

You'll now have to remove the test from the problem list.

-------------

Marked as reviewed by jlaskey (Reviewer).

PR: https://git.openjdk.org/jdk19/pull/60