initializer annotation on instance methods #2176

Merged
merged 5 commits into from Mar 29, 2016

Conversation

6 participants
@kohsuke
Member

kohsuke commented Mar 26, 2016

Improve the @Initializer annotation by allowing it to be on instance methods. This often eliminates the need of instance lookup via ExtensionList and others.

This PR also contains NioChannelSelector termination, which leaks NioChannelHub, which is what got me looking into this space in the first place. This problem manifests itself in unit tests that show tons of leaked threads.

Yes, I know, I know, one shouldn't put two changes in one PR.

@reviewbybees

kohsuke added some commits Mar 26, 2016

NioChannelHub must be explicitly shut down
... or else it'll keep running. This can be seen as a large number of zombie threads like the following in the unit test:

"NioChannelHub keys=0 gen=0: Computer.threadPoolForRemoting [#250]" daemon prio=10 tid=0x00007f793d841000 nid=0x3cc1 runnable [0x00007f78e2665000]
   java.lang.Thread.State: RUNNABLE
        at sun.nio.ch.EPollArrayWrapper.epollWait(Native Method)
        at sun.nio.ch.EPollArrayWrapper.poll(EPollArrayWrapper.java:228)
        at sun.nio.ch.EPollSelectorImpl.doSelect(EPollSelectorImpl.java:81)
        at sun.nio.ch.SelectorImpl.lockAndDoSelect(SelectorImpl.java:87)
        - locked <0x00000000c50d46d8> (a sun.nio.ch.Util$2)
        - locked <0x00000000c50d46e8> (a java.util.Collections$UnmodifiableSet)
        - locked <0x00000000c50d4690> (a sun.nio.ch.EPollSelectorImpl)
        at sun.nio.ch.SelectorImpl.select(SelectorImpl.java:98)
        at sun.nio.ch.SelectorImpl.select(SelectorImpl.java:102)
        at org.jenkinsci.remoting.nio.NioChannelHub.run(NioChannelHub.java:542)
        at jenkins.util.ContextResettingExecutorService$1.run(ContextResettingExecutorService.java:28)
        at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:471)
        at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:334)
        at java.util.concurrent.FutureTask.run(FutureTask.java:166)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
        at java.lang.Thread.run(Thread.java:722)
Allow instance methods for @Initializer and @Terminator
This simplifies typical usage of them.
The previous change requires that Guice is setup
... which is done by looking for locating & instantiating ExtensionFinders.
@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees Mar 26, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@stephenc

This comment has been minimized.

Show comment
Hide comment
@stephenc

stephenc Mar 26, 2016

Member

🐝 (though the NioChannelHub will be irrelevant once I land my JnlpProtocol4 implementation as SSLEngine is not compatible with the Hub, but I can make the other protocols use my new hub... Should also be able to adapt JnlpProtocol3 to work with my NIO Hub)

Member

stephenc commented Mar 26, 2016

🐝 (though the NioChannelHub will be irrelevant once I land my JnlpProtocol4 implementation as SSLEngine is not compatible with the Hub, but I can make the other protocols use my new hub... Should also be able to adapt JnlpProtocol3 to work with my NIO Hub)

@andresrc

This comment has been minimized.

Show comment
Hide comment
@andresrc

andresrc Mar 27, 2016

Contributor

🐝

Contributor

andresrc commented Mar 27, 2016

🐝

- RekeySecretAdminMonitor m = new RekeySecretAdminMonitor(); // throw-away instance
-
- FileBoolean flag = m.scanOnBoot;
+ public void scanOnReboot() throws InterruptedException, IOException, GeneralSecurityException {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Mar 28, 2016

Member

🐜 binary compatibility loss

@oleg-nenashev

oleg-nenashev Mar 28, 2016

Member

🐜 binary compatibility loss

This comment has been minimized.

@jglick

jglick Mar 28, 2016

Member

ditto

@jglick

jglick Mar 28, 2016

Member

ditto

@@ -27,7 +27,7 @@ public long getUptime() {
}
@Initializer(after=InitMilestone.JOB_LOADED)
- public static void init() {
- ExtensionList.lookup(Uptime.class).get(0).startTime = System.currentTimeMillis();
+ public void init() {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Mar 28, 2016

Member

🐜 Another compat loss. The method is not restricted

@oleg-nenashev

oleg-nenashev Mar 28, 2016

Member

🐜 Another compat loss. The method is not restricted

This comment has been minimized.

@jglick

jglick Mar 28, 2016

Member

So restrict it now. Can search GitHub to make sure it is unused.

@jglick

jglick Mar 28, 2016

Member

So restrict it now. Can search GitHub to make sure it is unused.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Mar 28, 2016

Member

There is a formal binary compatibility loss.
I don't see real use-cases for the methods in the external code, so I don't put a bug.
BTW, it would be preferable to handle the stuff in a compatible way (at least keep the deprecated stubs)

Member

oleg-nenashev commented Mar 28, 2016

There is a formal binary compatibility loss.
I don't see real use-cases for the methods in the external code, so I don't put a bug.
BTW, it would be preferable to handle the stuff in a compatible way (at least keep the deprecated stubs)

@@ -970,6 +970,11 @@ protected void runTask(Task task) throws Exception {
@Override
protected void onInitMilestoneAttained(InitMilestone milestone) {
initLevel = milestone;
+ if (milestone==PLUGINS_PREPARED) {
+ // set up Guice to enable injection as early as possible
+ // before this milestone, ExtensionList.ensureLoaded() won't actually try to locate instances

This comment has been minimized.

@jglick

jglick Mar 28, 2016

Member

Which makes me think this whole change is a bad idea, in that it would seem to make fixing JENKINS-22050 much harder.

@jglick

jglick Mar 28, 2016

Member

Which makes me think this whole change is a bad idea, in that it would seem to make fixing JENKINS-22050 much harder.

This comment has been minimized.

@kohsuke

kohsuke Mar 28, 2016

Member

I'm not sure if I understand. This hunk does have a hackish feel to it, but that's because of our two minds about Guice; on one hand it's "just" an ExtensionFinder like every other, yet it's also a critical dependency as can be seen in Jenkins.getInjector()

I don't think JENKINS-22050 is a problem, but even if it is, I'd think this change is going to make that fix easier by allowing the reactor to reason about what initializer methods need.

@kohsuke

kohsuke Mar 28, 2016

Member

I'm not sure if I understand. This hunk does have a hackish feel to it, but that's because of our two minds about Guice; on one hand it's "just" an ExtensionFinder like every other, yet it's also a critical dependency as can be seen in Jenkins.getInjector()

I don't think JENKINS-22050 is a problem, but even if it is, I'd think this change is going to make that fix easier by allowing the reactor to reason about what initializer methods need.

This comment has been minimized.

@jglick

jglick Mar 28, 2016

Member

I don't think JENKINS-22050 is a problem

I think it is; working on demonstrating it.

I'd think this change is going to make that fix easier by allowing the reactor to reason about what initializer methods need.

I hope so. In the meantime, the force-loading of components even earlier than we already do looks like a step in the wrong direction.

@jglick

jglick Mar 28, 2016

Member

I don't think JENKINS-22050 is a problem

I think it is; working on demonstrating it.

I'd think this change is going to make that fix easier by allowing the reactor to reason about what initializer methods need.

I hope so. In the meantime, the force-loading of components even earlier than we already do looks like a step in the wrong direction.

@kohsuke kohsuke merged commit a5bede8 into master Mar 29, 2016

1 check passed

Jenkins This pull request looks good
Details

@kohsuke kohsuke deleted the instance-initializer branch Mar 29, 2016

daniel-beck added a commit that referenced this pull request Apr 4, 2016

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