Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[JENKINS-60641, JENKINS-60644] incorrectly converted AntClassLoader.loadResource to a Java 5 for-loop #4419

Merged
merged 2 commits into from
Jan 6, 2020
Merged

[JENKINS-60641, JENKINS-60644] incorrectly converted AntClassLoader.loadResource to a Java 5 for-loop #4419

merged 2 commits into from
Jan 6, 2020

Conversation

jglick
Copy link
Member

@jglick jglick commented Jan 6, 2020 •
edited
Loading

JENKINS-60644

Corrects a mistake in #4254 which broke some plugins.

  • fix confirmed via ATH
  • fix confirmed here via JTH

Proposed changelog entries

  • Corrects a critical regression in Jenkins 2.212 affecting at least the script-security and active-directory plugins.

@jglick jglick added regression-fix Pull request that fixes a regression in one of the previous Jenkins releases major-bug For changelog: Major bug. Will be highlighted on the top of the changelog labels Jan 6, 2020
oleg-nenashev
oleg-nenashev approved these changes Jan 6, 2020
View reviewed changes
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching! I was unable to notice it in code reviews even after pin-pointing the pull request. Will need an out=of order release if @kohsuke is available

@oleg-nenashev oleg-nenashev changed the title #4254 incorrectly converted AntClassLoader.loadResource to a Java 5 for-loop [JENKINS-60641, JENKINS-60644] incorrectly converted AntClassLoader.loadResource to a Java 5 for-loop Jan 6, 2020
@oleg-nenashev oleg-nenashev requested a review from a team January 6, 2020 15:34
@zbynek
Copy link
Contributor

zbynek commented Jan 6, 2020

@jglick I guess the same fix is needed in the new getUrl method which also changed from iterating a vector to an arrayList

@jglick
Copy link
Member Author

jglick commented Jan 6, 2020

I guess the same fix is needed in the new getUrl method

No, that one looks fine to me, since it is checking for a null result in each iteration. The problem here in loadResource was that it was looping over all path components but actually ignoring everything but the last one.

@jglick
Reproduced error in functional test with a small sample plugin.
0767caa 
Without fix, Jenkins startup fails:
java.lang.NullPointerException
	at java.io.Reader.<init>(Reader.java:78)
	at java.io.InputStreamReader.<init>(InputStreamReader.java:113)
	at org.apache.commons.io.IOUtils.copy(IOUtils.java:2440)
	at org.apache.commons.io.IOUtils.toString(IOUtils.java:1084)
	at io.jenkins.plugins.loads_resource.Main.stuff(Main.java:36)
oleg-nenashev
oleg-nenashev approved these changes Jan 6, 2020
View reviewed changes
test/src/test/java/jenkins/util/AntClassLoaderTest.java
@WithPlugin("loads-resource.jpi")
@Test
public void loadsResource() throws Exception {
assertNotNull(r.jenkins.pluginManager.getPlugin("loads-resource").classLoader.getResourceAsStream("io/jenkins/plugins/loads_resource/stuff"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably the only case where PluginManager.getPlugin("loads-resource") actually works in JTH :(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it would always work for loaded plugins, but normally the classLoader would just be AppClassLoader. You need to go out of your way to test the real plugin loaders.

jglick
jglick commented Jan 6, 2020
View reviewed changes
core/src/main/java/jenkins/util/AntClassLoader.java
}
return stream;
return null;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For those who like this sort of thing:

return pathComponents.stream().map(pc -> getResourceStream(pc, name)).filter(Objects::nonNull).findFirst().orElse(null);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't

@jglick jglick added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jan 6, 2020
@jglick jglick merged commit 6c7339a into jenkinsci:master Jan 6, 2020
@jglick jglick deleted the loadResource-JENKINS-60644 branch January 6, 2020 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleg-nenashev oleg-nenashev oleg-nenashev approved these changes

@timja timja timja approved these changes

@jsoref jsoref Awaiting requested review from jsoref

Assignees
No one assigned
Labels
major-bug For changelog: Major bug. Will be highlighted on the top of the changelog ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback regression-fix Pull request that fixes a regression in one of the previous Jenkins releases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jglick @zbynek @oleg-nenashev @timja