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.

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

Findbugs: null pointer dereference fixes. #197

Merged
merged 3 commits into from Jan 15, 2015

Conversation

Projects
None yet
3 participants
@rsandell
Copy link
Member

commented Jan 12, 2015

The bump in core version introduced @checkfornull on Jenkins.getInstance
and escalated to PluginImpl.getInstance.
Added some static helper/shortcut methods in PluginImpl so not too many
new if statements littering the rest of the code.

This commit should fix all current, including old,
"possible null pointer dereference" warnings.

Findbugs: null pointer dereference fixes.
The bump in core version introduced @checkfornull on Jenkins.getInstance
and escalated to PluginImpl.getInstance.
Added some static helper/shortcut methods in PluginImpl so not too many
new if statements littering the rest of the code.

This commit should fix all current, including old,
"possible null pointer dereference" warnings.
@rsandell

This comment has been minimized.

Copy link
Member Author

commented Jan 12, 2015

Hoping to get some review help. Not sure I have all annotations set correctly for example...

@TWestling @jglick @stephenc @recampbell ?

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Jan 12, 2015

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@@ -138,7 +142,9 @@ public String getDisplayName() {
* @return the list of descriptors containing GerritServer's descriptor.
*/
public static DescriptorExtensionList<GerritServer, GerritServer.DescriptorImpl> serverDescriptorList() {
return Jenkins.getInstance()
Jenkins jenkins = Jenkins.getInstance();
assert jenkins != null;

This comment has been minimized.

Copy link
@jglick

jglick Jan 12, 2015

Member

Note that it is valid for Jenkins.getInstance to be null during shutdown, so in general you need to think about whether your code might be running in this mode and behave gracefully if so.

There is no good solution for DescriptorExtensionList. For ExtensionList you can use the new lookup method to behave smoothly during shutdown.

This comment has been minimized.

Copy link
@rsandell

rsandell Jan 13, 2015

Author Member

That depends on when Jenkins.getInstance() becomes null, is it before or after Plugin.stop()?

This comment has been minimized.

Copy link
@jglick

jglick Jan 13, 2015

Member

Not sure offhand.

if (jenkins != null) {
return jenkins.getPlugin(PluginImpl.class);
} else {
logger.error("INITIALIZATION Error, Jenkins could not be found, so no plugin!");

This comment has been minimized.

Copy link
@jglick

jglick Jan 12, 2015

Member

Probably better to just shut up.

ExtensionList<DependencyQueueTaskDispatcher> dispatchers =
Jenkins.getInstance().getExtensionList(DependencyQueueTaskDispatcher.class);
jenkins.getExtensionList(DependencyQueueTaskDispatcher.class);

This comment has been minimized.

Copy link
@jglick

jglick Jan 12, 2015

Member

Use ExtensionList.lookup if your baseline is sufficiently new.

This comment has been minimized.

Copy link
@rsandell

rsandell Jan 13, 2015

Author Member

The baseline is 1.565.3 which doesn't seem to have it.

GerritCause cause = getCause(r);
logger.debug("Completed. Build: {} Cause: {}", r, cause);
if (cause != null) {
if (r != null && cause != null) {

This comment has been minimized.

Copy link
@jglick

This comment has been minimized.

Copy link
@rsandell

rsandell Jan 13, 2015

Author Member

Minimal effort to get rid of the warning that r might be null.
The method in RunListener is declared as public void onCompleted(R r, @Nonnull TaskListener listener) {}

This comment has been minimized.

Copy link
@jglick

jglick Jan 13, 2015

Member

But in fact it should be @Nonnull Run. Can fix that later in core, but in the meantime I think you can just add this annotation to your override.

config = Setup.createConfig();
GerritServer server = new GerritServer(PluginImpl.DEFAULT_SERVER_NAME);
server.setConfig(config);
when(plugin.getServer(eq(PluginImpl.DEFAULT_SERVER_NAME))).thenReturn(server);
when(plugin.getFirstServer()).thenReturn(server);

This comment has been minimized.

Copy link
@jglick

jglick Jan 12, 2015

Member

Any particular reason you are deleting all this?

I guess because this test already uses JenkinsRule?

This comment has been minimized.

Copy link
@rsandell

rsandell Jan 13, 2015

Author Member

Yes, at some point in time it had become some strange abomination with mixed JenkinsRule and PowerMock, when the test didn't pass I started with simplifying the test code to understand what was going wrong in the tests and that fixed the issues.

@rsandell

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2015

Annotated onCompleted to simplify if
And one more small null pointer warning that was hiding behind another category.

rsandell added a commit that referenced this pull request Jan 15, 2015

Merge pull request #197 from jenkinsci/findbugs
Findbugs: null pointer dereference fixes.

@rsandell rsandell merged commit 03aa3ac into master Jan 15, 2015

1 check passed

Jenkins This pull request looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.