Switch Jenkins.getInstance() to @Nonnull #2297

Merged
merged 1 commit into from May 10, 2016

Conversation

5 participants
@batmat
Member

batmat commented Apr 28, 2016

As detailed by @stephenc:

So there is getInstanceOrNull() which is for use in Jenkins core or
in code that plugins add that may escape the Jenkins singleton
lifecycle.

In regular plugins getInstance() will never return null as the
lifecycle does not instantiate them until after there is a Jenkins
singleton and the plugins will be stopped before there is no
singleton.

The getActiveInstance() is therefore equivalent to getInstance()
and should never have been born.

The only way a plugin can get a null from Jenkins.getInstance() is
if you install an atExit() handler, use a PhantomReference type
thing, or directly manipulate the servlet container in some way or
other. A correctly written plugin should unwind any of those things
when it is stopped or a method bound to the termination lifecycle. If
there are some cases (for which I cannot anticipate a good coder being
able to write a correct termination lifecycle method to fix) where the
code cannot be unhooked... then in those cases the plugin should use
getInstanceOrNull() and know how to respond to the null that could
not be avoided.

@batmat

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Apr 28, 2016

Member

I'm not sure I follow the idea. Do we want to break the compatibility in 2.x? The method may be used in the code outside plugins (e.g. Boot hook scripts), hence I'm still not convinced it's safe.

Member

oleg-nenashev commented Apr 28, 2016

I'm not sure I follow the idea. Do we want to break the compatibility in 2.x? The method may be used in the code outside plugins (e.g. Boot hook scripts), hence I'm still not convinced it's safe.

batmat pushed a commit to batmat/jenkins-getinstance-tester that referenced this pull request Apr 29, 2016

@batmat

This comment has been minimized.

Show comment
Hide comment
@batmat

batmat Apr 29, 2016

Member

Not absolutely sure what you have in mind with "boot hook scripts" @oleg-nenashev.
If you talk about init.d groovy scripts to be called during startup, then I created https://github.com/batmat/jenkins-getinstance-tester to check.

This shows that even in that case Jenkins.getInstance() is not null. (well, as it's used generally to configure plugins, that's a good news. So possibly, you're talking of something else, now I think of it again).

Apr 29, 2016 1:26:27 PM org.jenkinsci.main.modules.sshd.SSHD start
INFO: Started SSHD at port 42168
Well, OK, getInstance() did not return null.
Apr 29, 2016 1:26:27 PM jenkins.util.groovy.GroovyHookScript execute
Member

batmat commented Apr 29, 2016

Not absolutely sure what you have in mind with "boot hook scripts" @oleg-nenashev.
If you talk about init.d groovy scripts to be called during startup, then I created https://github.com/batmat/jenkins-getinstance-tester to check.

This shows that even in that case Jenkins.getInstance() is not null. (well, as it's used generally to configure plugins, that's a good news. So possibly, you're talking of something else, now I think of it again).

Apr 29, 2016 1:26:27 PM org.jenkinsci.main.modules.sshd.SSHD start
INFO: Started SSHD at port 42168
Well, OK, getInstance() did not return null.
Apr 29, 2016 1:26:27 PM jenkins.util.groovy.GroovyHookScript execute
@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 3, 2016

Member

I also need to dig into the code

Member

oleg-nenashev commented May 3, 2016

I also need to dig into the code

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck May 3, 2016

Member

Tentative 👍 here, but more @jenkinsci/code-reviewers would be welcome.

Member

daniel-beck commented May 3, 2016

Tentative 👍 here, but more @jenkinsci/code-reviewers would be welcome.

@stephenc

This comment has been minimized.

Show comment
Hide comment
@stephenc

stephenc May 4, 2016

Member

I'm 👍 though it might be better to have a system property control the exact behaviour for 10-20 versions so that it's annotatied @NonNull but it throwing the ISE can be turned off by a system property to allow users an escape hatch if they have a valid code path (or even an invalid one) that needs addressing somehow

Member

stephenc commented May 4, 2016

I'm 👍 though it might be better to have a system property control the exact behaviour for 10-20 versions so that it's annotatied @NonNull but it throwing the ISE can be turned off by a system property to allow users an escape hatch if they have a valid code path (or even an invalid one) that needs addressing somehow

@stephenc

This comment has been minimized.

Show comment
Hide comment
@stephenc

stephenc May 4, 2016

Member

(to be clear I would have the default throw the ISE)

Member

stephenc commented May 4, 2016

(to be clear I would have the default throw the ISE)

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck May 4, 2016

Member

Right, we're approaching 2.x LTS now, so that would be a useful addition.

Member

daniel-beck commented May 4, 2016

Right, we're approaching 2.x LTS now, so that would be a useful addition.

@batmat

This comment has been minimized.

Show comment
Hide comment
@batmat

batmat May 4, 2016

Member

Right. Good idea. Will be able to do that on Sunday evening.
Le 4 mai 2016 2:37 PM, "Daniel Beck" notifications@github.com a écrit :

Right, we're approaching 2.x LTS now, so that would be a useful addition.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#2297 (comment)

Member

batmat commented May 4, 2016

Right. Good idea. Will be able to do that on Sunday evening.
Le 4 mai 2016 2:37 PM, "Daniel Beck" notifications@github.com a écrit :

Right, we're approaching 2.x LTS now, so that would be a useful addition.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#2297 (comment)

@batmat

This comment has been minimized.

Show comment
Hide comment
@batmat

batmat May 8, 2016

Member

@stephenc Just amended the version using a sysprop as you asked rightly.
And also, yes, it's throwing an exception by default as it's the very goal of that change indeed.

Ping @daniel-beck @oleg-nenashev also for a second look, and @jenkinsci/code-reviewers in general since this (small) change needs eyes.

Member

batmat commented May 8, 2016

@stephenc Just amended the version using a sysprop as you asked rightly.
And also, yes, it's throwing an exception by default as it's the very goal of that change indeed.

Ping @daniel-beck @oleg-nenashev also for a second look, and @jenkinsci/code-reviewers in general since this (small) change needs eyes.

@stephenc

This comment has been minimized.

Show comment
Hide comment
@stephenc

stephenc May 8, 2016

Member

👍

Member

stephenc commented May 8, 2016

👍

@batmat batmat added the needs-review label May 8, 2016

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck May 9, 2016

Member

👍

Member

daniel-beck commented May 9, 2016

👍

@batmat batmat added ready-for-merge and removed needs-review labels May 9, 2016

@batmat

This comment has been minimized.

Show comment
Hide comment
@batmat

batmat May 9, 2016

Member

Will merge later today or tomorrow if nobody objects in the meantime.

Member

batmat commented May 9, 2016

Will merge later today or tomorrow if nobody objects in the meantime.

+ if(!Boolean.getBoolean(Jenkins.class.getName()+".disableExceptionOnNullInstance")) {
+ // remove that second block around 2.20 (that is: ~20 versions to battle test it)
+ // See https://github.com/jenkinsci/jenkins/pull/2297#issuecomment-216710150
+ throw new IllegalStateException();

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 9, 2016

Member

Would be useful to restore the original text then

@oleg-nenashev

oleg-nenashev May 9, 2016

Member

Would be useful to restore the original text then

This comment has been minimized.

@batmat

batmat May 9, 2016

Member

Can you elaborate @oleg-nenashev ? Not sure which original text you're talking about.

// TODO throw an IllegalStateException in Jenkins 2.0+ ? In which case I don't understand why it would still apply. Or do you mean I should leave the TODO word?

@batmat

batmat May 9, 2016

Member

Can you elaborate @oleg-nenashev ? Not sure which original text you're talking about.

// TODO throw an IllegalStateException in Jenkins 2.0+ ? In which case I don't understand why it would still apply. Or do you mean I should leave the TODO word?

This comment has been minimized.

@stephenc

stephenc May 9, 2016

Member

I think he wants

throw new IllegalStateException("Jenkins has not been started, or was already shut down");
@stephenc

stephenc May 9, 2016

Member

I think he wants

throw new IllegalStateException("Jenkins has not been started, or was already shut down");

This comment has been minimized.

@batmat

batmat May 9, 2016

Member

Right, like in current getActiveInstance().
Fixed.

@batmat

batmat May 9, 2016

Member

Right, like in current getActiveInstance().
Fixed.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 9, 2016

Member

IMHO needs an exception text to be restored, hence weak 👎 . Minor fix BTW

Member

oleg-nenashev commented May 9, 2016

IMHO needs an exception text to be restored, hence weak 👎 . Minor fix BTW

+ Jenkins instance = HOLDER.getInstance();
+ if (instance == null) {
+ if(!Boolean.getBoolean(Jenkins.class.getName()+".disableExceptionOnNullInstance")) {
+ // remove that second block around 2.20 (that is: ~20 versions to battle test it)

This comment has been minimized.

@stephenc

stephenc May 9, 2016

Member

This should have TODO so we find it again

@stephenc

stephenc May 9, 2016

Member

This should have TODO so we find it again

This comment has been minimized.

@batmat

batmat May 9, 2016

Member

Done.

@batmat

batmat May 9, 2016

Member

Done.

Switch Jenkins.getInstance() to @nonnull
As detailed by @stephenc:

> So there is `getInstanceOrNull()` which is for use in Jenkins core or
> in code that plugins add that may escape the Jenkins singleton
> lifecycle.
>
> In regular plugins `getInstance()` will never return null as the
> lifecycle does not instantiate them until after there is a Jenkins
> singleton and the plugins will be stopped before there is no
> singleton.
>
> The `getActiveInstance()` is therefore equivalent to `getInstance()`
> and should never have been born.
>
> The only way a plugin can get a `null` from `Jenkins.getInstance()` is
> if you install an `atExit()` handler, use a `PhantomReference` type
> thing, or directly manipulate the servlet container in some way or
> other. A correctly written plugin should unwind any of those things
> when it is stopped or a method bound to the termination lifecycle. If
> there are some cases (for which I cannot anticipate a good coder being
> able to write a correct termination lifecycle method to fix) where the
> code cannot be unhooked... then in those cases the plugin should use
> `getInstanceOrNull()` and know how to respond to the `null` that could
> not be avoided.
@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
Member

oleg-nenashev commented May 9, 2016

👍

@batmat

This comment has been minimized.

Show comment
Hide comment
@batmat

batmat May 9, 2016

Member

@oleg-nenashev Fixed, thanks for the review.

Member

batmat commented May 9, 2016

@oleg-nenashev Fixed, thanks for the review.

@batmat

This comment has been minimized.

Show comment
Hide comment
@batmat

batmat May 9, 2016

Member

Lol, commenting at the very same moment. o/ @oleg-nenashev ;-)

Member

batmat commented May 9, 2016

Lol, commenting at the very same moment. o/ @oleg-nenashev ;-)

@batmat

This comment has been minimized.

Show comment
Hide comment
@batmat

batmat May 9, 2016

Member

Will merge after the build completes successfully.

Thanks to everyone.

Member

batmat commented May 9, 2016

Will merge after the build completes successfully.

Thanks to everyone.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 9, 2016

Member

@batmat A kind of lifehack: Merge&Squash also allows to avoid a adding a merge commit for single-commit PR. Makes the history shorter: https://github.com/jenkinsci/jenkins/commits/master

/me hides before he starts getting grumbles from non-squash fans

Member

oleg-nenashev commented May 9, 2016

@batmat A kind of lifehack: Merge&Squash also allows to avoid a adding a merge commit for single-commit PR. Makes the history shorter: https://github.com/jenkinsci/jenkins/commits/master

/me hides before he starts getting grumbles from non-squash fans

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck May 9, 2016

Member

@oleg-nenashev You fix the changelog after every release then. Too much crap to sift through otherwise, having commits titled Merge pull request speeds that up considerably for me.

Member

daniel-beck commented May 9, 2016

@oleg-nenashev You fix the changelog after every release then. Too much crap to sift through otherwise, having commits titled Merge pull request speeds that up considerably for me.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 9, 2016

Member

@daniel-beck Yes, that's a valid point. May also help if we start squashing commits, but oh well

Member

oleg-nenashev commented May 9, 2016

@daniel-beck Yes, that's a valid point. May also help if we start squashing commits, but oh well

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 9, 2016

Member

@daniel-beck But I've got the point that I should not forget about changelogs for PRs I merge. My impression was that it's better to process it before the release because somebody has to do it in any case. :(

Member

oleg-nenashev commented May 9, 2016

@daniel-beck But I've got the point that I should not forget about changelogs for PRs I merge. My impression was that it's better to process it before the release because somebody has to do it in any case. :(

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck May 9, 2016

Member

@oleg-nenashev Yep, and that someone has dozens of commits to sift through. If every PR has a merge commit with the generated message, that makes it much easier. But let's move this topic elsewhere. Just saying, there are valid reasons to not squash unless done consistently every time.

Member

daniel-beck commented May 9, 2016

@oleg-nenashev Yep, and that someone has dozens of commits to sift through. If every PR has a merge commit with the generated message, that makes it much easier. But let's move this topic elsewhere. Just saying, there are valid reasons to not squash unless done consistently every time.

@batmat batmat merged commit b9bb52f into jenkinsci:master May 10, 2016

1 check passed

Jenkins This pull request looks good
Details
@batmat

This comment has been minimized.

Show comment
Hide comment
@batmat

batmat May 10, 2016

Member

Gonna modify the changelog.

Member

batmat commented May 10, 2016

Gonna modify the changelog.

@batmat batmat deleted the batmat:getInstanceNonnull branch May 10, 2016

batmat added a commit to batmat/jenkins that referenced this pull request May 10, 2016

@batmat

This comment has been minimized.

Show comment
Hide comment
@batmat

batmat May 10, 2016

Member

@daniel-beck is that how it must be done batmat@2b6fb33?
(Mostly the format, I will re-check the content later today, was a bit in a hurry just now).

(It's on my fork to check first)

Then IIRC, I just push that onto the jenkins one, w/o PR right?

Thanks

Member

batmat commented May 10, 2016

@daniel-beck is that how it must be done batmat@2b6fb33?
(Mostly the format, I will re-check the content later today, was a bit in a hurry just now).

(It's on my fork to check first)

Then IIRC, I just push that onto the jenkins one, w/o PR right?

Thanks

@batmat

This comment has been minimized.

Show comment
Hide comment
@batmat

batmat May 16, 2016

Member

🔥 JENKINS-34857

TL;DR: someone started having issue with this PR on Jenkins 2.4 released yesterday... So possibly a urgent revert + release may be needed. Mainly because (still supposedly at this stage of analysis) the code is running in an agent hence the sysprop isn't present there :-/.

The associated crashing code for reference: https://github.com/jenkinsci/subversion-plugin/blob/3616a6a548800203c2399b2d0a97e8fba978b44e/src/main/java/hudson/scm/SubversionSCM.java#L2544 (cc @recena)

Also, ping @stephenc for your feedback.

Member

batmat commented May 16, 2016

🔥 JENKINS-34857

TL;DR: someone started having issue with this PR on Jenkins 2.4 released yesterday... So possibly a urgent revert + release may be needed. Mainly because (still supposedly at this stage of analysis) the code is running in an agent hence the sysprop isn't present there :-/.

The associated crashing code for reference: https://github.com/jenkinsci/subversion-plugin/blob/3616a6a548800203c2399b2d0a97e8fba978b44e/src/main/java/hudson/scm/SubversionSCM.java#L2544 (cc @recena)

Also, ping @stephenc for your feedback.

@stephenc

This comment has been minimized.

Show comment
Hide comment
@stephenc

stephenc May 16, 2016

Member

That is indicative of a real issue in the subversion plugin. Plugins should not be pulling the Jenkins class over remoting. IIRC we had plans to block that at some point in the future.

Member

stephenc commented May 16, 2016

That is indicative of a real issue in the subversion plugin. Plugins should not be pulling the Jenkins class over remoting. IIRC we had plans to block that at some point in the future.

@recena

This comment has been minimized.

Show comment
Hide comment
@recena

recena May 16, 2016

Contributor

<ironic mode=on>Without any doubt this is the best way to find out a bug.</ironic>

Contributor

recena commented May 16, 2016

<ironic mode=on>Without any doubt this is the best way to find out a bug.</ironic>

@batmat

This comment has been minimized.

Show comment
Hide comment
@batmat

batmat May 17, 2016

Member

Possibly @recena the short term "fix" could be to call Jenkins.getInstanceOrNull(). But the issue is it's only starting at 1.657, so you may want to call this method by reflection, and then fall back onto Jenkins.getInstance() if not found. Not beautiful, but should prevent the SVN plugin to crash w/ 2.4 (or if we reintroduce the exception in the future).

Member

batmat commented May 17, 2016

Possibly @recena the short term "fix" could be to call Jenkins.getInstanceOrNull(). But the issue is it's only starting at 1.657, so you may want to call this method by reflection, and then fall back onto Jenkins.getInstance() if not found. Not beautiful, but should prevent the SVN plugin to crash w/ 2.4 (or if we reintroduce the exception in the future).

@stephenc

This comment has been minimized.

Show comment
Hide comment
@stephenc

stephenc May 17, 2016

Member

Well in actual fact that method could be better served as

@CheckForNull
    private static DescriptorImpl descriptor() {
        return ExtensionList.lookup(Descriptor.class).get(DescriptorImpl.class);
    }

But more critically, you should not be looking up the descriptor in a remote callable

Member

stephenc commented May 17, 2016

Well in actual fact that method could be better served as

@CheckForNull
    private static DescriptorImpl descriptor() {
        return ExtensionList.lookup(Descriptor.class).get(DescriptorImpl.class);
    }

But more critically, you should not be looking up the descriptor in a remote callable

@stephenc

This comment has been minimized.

Show comment
Hide comment
@stephenc

stephenc May 17, 2016

Member

Plus https://github.com/jenkinsci/subversion-plugin/blob/master/src/main/java/hudson/scm/SubversionSCM.java#L1041 is going to be pulling too much code over to the slave. Instead the options should be created https://github.com/jenkinsci/subversion-plugin/blob/master/src/main/java/hudson/scm/SubversionSCM.java#L948 and modified for the fact that they are going to be for the remote agent...

Basically the Subversion plugin is doing this all wrong and causing unnecessary classloading into the slaves which may be having other impacts that have remained hidden from us

Member

stephenc commented May 17, 2016

Plus https://github.com/jenkinsci/subversion-plugin/blob/master/src/main/java/hudson/scm/SubversionSCM.java#L1041 is going to be pulling too much code over to the slave. Instead the options should be created https://github.com/jenkinsci/subversion-plugin/blob/master/src/main/java/hudson/scm/SubversionSCM.java#L948 and modified for the fact that they are going to be for the remote agent...

Basically the Subversion plugin is doing this all wrong and causing unnecessary classloading into the slaves which may be having other impacts that have remained hidden from us

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment