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-62043] Fix NPE which broke Jenkins builds #452

Merged
merged 4 commits into from
Apr 29, 2020
Merged

[JENKINS-62043] Fix NPE which broke Jenkins builds #452

merged 4 commits into from
Apr 29, 2020

Conversation

roundtrip-rslobojan
Copy link
Contributor

In PR #433, which was released in AWS EC2 Plugin 1.50, a bug was introduced due to improper
handling of an important transient field which is accessed very frequently by the Jenkins server
due to it being referenced in EC2AbstractSlave.isAcceptingTasks(). This broke any Jenkins
user which had running EC2 Nodes during the update to plugin 1.50 (see
https://issues.jenkins-ci.org/browse/JENKINS-62043 for details).

@roundtrip-rslobojan
[JENKINS-62043] Fix NPE which broke Jenkins builds
4cfa060 
In PR #433, which was released in AWS EC2 Plugin 1.50, a bug was introduced due to improper handling of an
important transient field which is accessed very frequently by the Jenkins server due to it being referenced in
`EC2AbstractSlave.isAcceptingTasks()`. This broke any Jenkins user which had running EC2 Nodes during the
update to plugin 1.50 (see https://issues.jenkins-ci.org/browse/JENKINS-62043 for details).
@nigelarmstrong-bc
Copy link
Contributor

This looks like this should work. @res0nance @jvz @fcojfernandez

@roundtrip-rslobojan
Copy link
Contributor Author

Spurious failure from Spotbugs - it's wrong about this one:

[2020-04-27T20:48:44.048Z] [INFO] --- spotbugs-maven-plugin:3.1.12.2:check (spotbugs) @ ec2 ---
[2020-04-27T20:48:44.048Z] [INFO] BugInstance size is 1
[2020-04-27T20:48:44.048Z] [INFO] Error size is 0
[2020-04-27T20:48:44.048Z] [INFO] Total bugs: 1
[2020-04-27T20:48:44.048Z] [ERROR] Redundant nullcheck of hudson.plugins.ec2.EC2AbstractSlave.terminateScheduled, which is known to be non-null in hudson.plugins.ec2.EC2AbstractSlave.readResolve() [hudson.plugins.ec2.EC2AbstractSlave] Redundant null check at EC2AbstractSlave.java:[line 214] RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE

Going to update the PR to make it happy. 😝

@roundtrip-rslobojan
Excluded false-positive nullcheck Spotbugs rule
1552e0d 
Turned off a Spotbugs rule which incorrectly understands how Java serialization works, and which thought a transient field
couldn't be null in Object.readResolve() when it actually can be.
@roundtrip-rslobojan
Copy link
Contributor Author

Unable to request a PR review directly due to permissions, but this is a fix for a blocker issue with the 1.50 release of the AWS EC2 plugin @res0nance @jvz @fcojfernandez @thoulen - please see https://issues.jenkins-ci.org/browse/JENKINS-62043

res0nance
res0nance requested changes Apr 28, 2020
View reviewed changes
spotbugs-exclude.xml Outdated
@@ -6,4 +6,9 @@
<Bug pattern="RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE"/>
</Or>
</Match>
<Match>
Copy link
Contributor

Choose a reason for hiding this comment

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

You should revert changes to this file and instead remove

ec2-plugin/src/main/java/hudson/plugins/ec2/EC2AbstractSlave.java

Line 116 in 1552e0d

@Nonnull

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will give that a shot and see if Spotbugs is happy with that

@roundtrip-rslobojan
Removed @nonnull annotation from transient field
876ae4c 
Removed a javax.annotation.Nonnull annotation from a transient field to silence a Spotbugs warning which
incorrectly flagged a field as never being null when it can in fact be null due to Java serialization semantics
@roundtrip-rslobojan
Removed unneeded NONNULL exclusion
8f7f814 
Code in EC2AbstractSlave was updated to remove a javax.annotation.Nonnull annotation, so this Spotbugs warning should
no longer trigger and we can remove the exclusion.
@roundtrip-rslobojan
Copy link
Contributor Author

@res0nance thanks! That suggestion cleaned things up!

Who has write permissions and can merge this into the downstream repository and cut a new AWS EC2 Plugin release? I'm assuming we would want to call this 1.50.1 since it's an emergency bugfix to 1.50

@res0nance
Copy link
Contributor

@res0nance thanks! That suggestion cleaned things up!

Who has write permissions and can merge this into the downstream repository and cut a new AWS EC2 Plugin release? I'm assuming we would want to call this 1.50.1 since it's an emergency bugfix to 1.50

I can cut a new release, I'm trying to get one other person's eyes on this before making a new release

@thoulen thoulen assigned thoulen and unassigned thoulen Apr 28, 2020
@thoulen thoulen self-requested a review April 28, 2020 03:44
@nigelarmstrong-bc
Copy link
Contributor

After this goes in can we also get some serialization/ deserialization tests? I have no idea where to start, so maybe someone could write a test that would catch this.

@res0nance res0nance merged commit 08c32fe into jenkinsci:master Apr 29, 2020
@roundtrip-rslobojan roundtrip-rslobojan deleted the patch-1 branch April 29, 2020 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thoulen thoulen thoulen approved these changes

@res0nance res0nance res0nance approved these changes

@nigelarmstrong-bc nigelarmstrong-bc nigelarmstrong-bc approved these changes

@jvz jvz Awaiting requested review from jvz

@fcojfernandez fcojfernandez Awaiting requested review from fcojfernandez

Assignees
No one assigned
Labels
bugfix
Projects
None yet
Milestone
No milestone
4 participants
@roundtrip-rslobojan @nigelarmstrong-bc @res0nance @thoulen