IntelliJ/Java: Duplicate code -- Refactored as AntClassLoader.storeAndConvert(...Convert<T>)

[jsoref]

This is for discussion.

Notes:

• Commits was probably created by IntelliJ for Java cleanup #4034
• In general, commits should not change program behavior at all

Proposed changelog entries

• Internal: Internal Java code cleanup

Submitter checklist

• JIRA issue is well described
• Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
  * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
• Appropriate autotests or explanation to why this change has no tests
• For dependency updates: links to external changelogs and, if possible, full diffs

Maintainer checklist

Before the changes are marked as ready-for-merge:

• There are at least 2 approvals for the pull request and no outstanding requests for change
• Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
• Changelog entries in the PR title and/or Proposed changelog entries are correct
• Proper changelog labels are set so that the changelog can be generated automatically
• If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
• If it would make sense to backport the change to LTS, a JIRA issue should exist and be labeled as lts-candidate

Desired reviewers

@Wadeck

[oleg-nenashev]

Looks like there was misguiding comment in the code

[oleg-nenashev]

Does this comment really represent the logic? it was there for one code part, but not for another one. For me it looks like there was missing TODO here

[jsoref]

I think that this code is how it tries to eliminate a race condition.

The simplest racing logic would be:

 if (jarFile == null) {
            if (file.exists()) {
                jarFile = new JarFile(file);
                jarFiles.put(file, jarFile);
            } else {
                return null;
            }
// at this point, you'd expect `jarFile == (JarFile) jarFiles.get(file)`, but it might not if you win the race.
}

The correct thing to do is to lock something. Otherwise, even this code probably isn't good enough.

I think w/ this code and 3 threads we could still get a race:

Enter the if:

(1) T1  if (jarFile == null) {
(2) T2  if (jarFile == null) {
(3) T3  if (jarFile == null) {

Create a file:

(4) T1                 jarFile = new JarFile(file);
(5) T2                 jarFile = new JarFile(file);
(6) T3                jarFile = new JarFile(file);

Race time:

(7) T1                jarFiles.put(file, jarFile);
(8) T2                jarFiles.put(file, jarFile);
(9) T1           jarFile = (JarFile) jarFiles.get(file);
(10) T2           jarFile = (JarFile) jarFiles.get(file);
(11) T3                jarFiles.put(file, jarFile);
(12) T3           jarFile = (JarFile) jarFiles.get(file);

Check our winner's:

(13) T1        JarEntry entry = jarFile.getJarEntry(resourceName);
This is T2's jarFile from (5)/(8) -- retrieved at (9)
(14) T2        JarEntry entry = jarFile.getJarEntry(resourceName);
This is T2's jarFile from (5)/(8) -- stored at (8) and retrieved at (10)
(15) T3        JarEntry entry = jarFile.getJarEntry(resourceName);
This is T3's jarFile from (6) -- stored at (11) and retrieved at (12)

[jsoref]

I do not want to change the logic in this commit / PR. Changing logic should not really be done in refactoring.

It makes it really hard to review / look back at things retroactively / debug regressions / etc.

[alecharp]

The code needs some fixing to pass the compilation.

[oleg-nenashev]

I am fine with the new version, thanks!

[oleg-nenashev]

I plan to merge it tomorrow if no negative feedback. CC @jenkinsci/core-pr-reviewers

[MRamonLeon]

@jsoref could you please fix the conflicts and we will propose it for merge?

[timja]

@jsoref could you please fix the conflicts and we will propose it for merge?

fixed, was just a tiny import issue

[timja]

let's give it another build