search

jenkinsci / hipchat-plugin

HipChat notification plugin for Jenkins
https://plugins.jenkins.io/hipchat/
54 stars 85 forks source link

Adding a try catch for an NPE when collecting a change log #55

Closed dimacus closed 8 years ago

dimacus commented 9 years ago

From time to time, when executing a build for a PR, or when merging a PR into master, the plugin cannot find the individual author's Display name triggering an NPE and failing the build. This commit surrounds the change log collection step with a try/catch for an NPE and returns a null back to the caller.

Stack trace from the issues we have seen:

java.lang.NullPointerException
    at jenkins.plugins.hipchat.NotificationTypeUtils.getChanges(NotificationTypeUtils.java:45)
    at jenkins.plugins.hipchat.NotificationType.collectParametersFor(NotificationType.java:122)
    at jenkins.plugins.hipchat.NotificationType.getMessage(NotificationType.java:92)
    at jenkins.plugins.hipchat.HipChatNotifier.publishNotificationIfEnabled(HipChatNotifier.java:221)
    at jenkins.plugins.hipchat.HipChatNotifier.perform(HipChatNotifier.java:204)
    at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
    at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:779)
    at hudson.model.AbstractBuild$AbstractBuildExecution.performAllBuildSteps(AbstractBuild.java:726)
    at hudson.model.Build$BuildExecution.cleanUp(Build.java:195)
    at hudson.model.Run.execute(Run.java:1788)
    at com.groupon.jenkins.dynamic.build.DynamicBuild.run(DynamicBuild.java:96)
    at hudson.model.ResourceController.execute(ResourceController.java:98)
    at hudson.model.Executor.run(Executor.java:381)
Build step 'HipChat Notifications' marked build as failure
aldaris commented 9 years ago

Catching NullPointerException doesn't really sound like a good idea. Can you determine what exactly is null in the affected code? My best guess would be getAuthor() returning null, but then its JavaDoc says that it should never return null.. What kind of SCM are you using to trigger this? Note that even if the display name would be null, that should be working fine, so something else must be null in there.

dimacus commented 9 years ago

It's definitely getAuthor() the dispaly name. Using GitHub with the WebHook to trigger the build, and the Git plugin to download the code to the node.

There are a couple of possible situations that might be triggering this:

1) A pull request that has multiple commits by user A but was pushed/submitted by user B 2) I've noticed that there are differences in how GitHub treats the author of a commit if the user does a commit/push from machine vs edit commit via the HTML GUI. 3) Miss match between the GHE user and Jenkins user. What if the SCM plugin cannot find the user because he/she never created a Jenkins account, but since Jenkins authenticates against AD it is a valid user who's profile was never created. And if that user logs in this might be fixed?

Honestly after this patch the build that I had finally started to work again, but I can't justify to my boss digging deeply into the Git SCM plugin and figuring out why the never null parameter turns out to be null.

aldaris commented 8 years ago

Looking at the Git client plugin there may be some edge cases when the getAuthor can return null, can you tell what setting you have in the Global configuration for "Create new accounts base on author/committer's email"? If by any chance you still have an affected build lying around, if you could provide the changelog.xml for that build, that would be also very helpful (so that a bug can be raised against the git client plugin. Until then, I'll add in a null check for getAuthor.

dimacus commented 8 years ago

Sorry I don't have a good understanding of the bug to reliably replicate. But I think it's between GHE and git plugin.

If I'll ever figure it out, I'll send a PR to hit plugin, but in mean time a catch for null should suffice

Sent via Payphone

On Nov 1, 2015, at 4:03 AM, Peter Major notifications@github.com wrote:

Looking at the Git client plugin there may be some edge cases when the getAuthor can return null, can you tell what setting you have in the Global configuration for "Create new accounts base on author/committer's email"? If by any chance you still have an affected build lying around, if you could provide the changelog.xml for that build, that would be also very helpful (so that a bug can be raised against the git client plugin. Until then, I'll add in a null check for getAuthor.

— Reply to this email directly or view it on GitHub.