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

Fix for JENKINS-23152 #193

Merged
merged 17 commits into from Dec 22, 2014

Conversation

Projects
None yet
5 participants
@rsandell
Copy link
Member

commented Dec 15, 2014

Bulk of the fix is to move the event listener part out of the trigger and only reference the job by name and do a lookup of the job instance by name when needed.
This should let old instances of the jobs and triggers to be removed correctly and not have any duplicates cause trouble.

jglick and others added some commits May 22, 2014

Reproduced problem in test: old copy of project is still referenced (…
…see below); and second build is not run but not found.

private static com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.GerritTriggerTimer com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.GerritTriggerTimer.instance->
com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.GerritTriggerTimer@54f3e538-timer->
java.util.Timer@692dff13-queue->
java.util.TaskQueue@548b26c6-queue->
[Ljava.util.TimerTask;@729e2285-[2]->
com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.GerritTriggerTimerTask@7c6dae73-gerritTrigger->
com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.GerritTrigger@3248ded0-myProject->
hudson.model.FreeStyleProject@39913454
Progress. New leak:
private static com.sonyericsson.hudson.plugins.gerrit.trigger.PluginImpl com.sonyericsson.hudson.plugins.gerrit.trigger.PluginImpl.instance->
com.sonyericsson.hudson.plugins.gerrit.trigger.PluginImpl@75a69570-gerritEventManager->
com.sonyericsson.hudson.plugins.gerrit.trigger.GerritHandlerLifecycle@5d4a7b45-gerritEventListeners->
java.util.concurrent.CopyOnWriteArraySet@3579f74a-al->
java.util.concurrent.CopyOnWriteArrayList@11103fa6-array->
[Ljava.lang.Object;@73bcafa1-[2]->
com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.GerritTrigger@10642995-job->
hudson.model.FreeStyleProject@63dd2ec
Moving right along. Now getting:
private static com.sonyericsson.hudson.plugins.gerrit.trigger.gerritnotifier.ToGerritRunListener com.sonyericsson.hudson.plugins.gerrit.trigger.gerritnotifier.ToGerritRunListener.instance->
com.sonyericsson.hudson.plugins.gerrit.trigger.gerritnotifier.ToGerritRunListener@7127c1a1-memory->
com.sonyericsson.hudson.plugins.gerrit.trigger.gerritnotifier.model.BuildMemory@4cbf8111-memory->
java.util.TreeMap@26f10c06-root->
java.util.TreeMap$Entry@568b866f-value->
com.sonyericsson.hudson.plugins.gerrit.trigger.gerritnotifier.model.BuildMemory$MemoryImprint@4ab70a17-list->
java.util.ArrayList@28967588-elementData->
[Ljava.lang.Object;@3c9b3ade-[0]->
com.sonyericsson.hudson.plugins.gerrit.trigger.gerritnotifier.model.BuildMemory$MemoryImprint$Entry@23ac0ff5-project->
hudson.model.FreeStyleProject@3f5c583e
Down to something that looks more like a test artifact than anything …
…else.

private static org.jvnet.hudson.test.TestEnvironment org.jvnet.hudson.test.TestEnvironment.CURRENT->
org.jvnet.hudson.test.TestEnvironment@637edfa7-testCase->
com.sonyericsson.hudson.plugins.gerrit.trigger.spec.SpecGerritTriggerHudsonTest@52c74dec-server->
org.mortbay.jetty.Server@5c420b3d-_sessionIdManager->
org.mortbay.jetty.servlet.HashSessionIdManager@ed5f6a8-_sessions->
org.mortbay.util.MultiMap@376ca809-table->
[Ljava.util.HashMap$Entry;@6dee3245-[8]->
java.util.HashMap$Entry@2e3e2c34-value->
org.mortbay.jetty.servlet.HashSessionManager$Session@78cd92a2-_values->
java.util.HashMap@4a7e47af-table->
[Ljava.util.HashMap$Entry;@647d7bd5-[2]->
java.util.HashMap$Entry@51b14c9-value->
org.kohsuke.stapler.bind.BoundObjectTable$Table@46ae9e11-entries->
java.util.HashMap@26724211-table->
[Ljava.util.HashMap$Entry;@3600505a-[9]->
java.util.HashMap$Entry@25660079-value->
org.kohsuke.stapler.bind.BoundObjectTable$StrongRef@70c97fa5-o->
hudson.widgets.RenderOnDemandClosure@4283613-variables->
hudson.util.PackedMap@62b6b956-kvpairs->
[Ljava.lang.Object;@5d630352-[1]->
hudson.model.FreeStyleProject@61b65bb0
Test still failing (and heap walker broken) and still have old instan…
…ce in memory, yet bug no longer reproducible live.

hprof heap walk shows this root ref:
this     - value: hudson.model.FreeStyleProject #1
 <- project     - class: com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.data.TriggeredItemEntity, value: hudson.model.FreeStyleProject #1
  <- item     - class: java.util.LinkedList$Node, value: com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.data.TriggeredItemEntity #1
   <- last     - class: java.util.LinkedList, value: java.util.LinkedList$Node #1237
    <- builds     - class: com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.actions.manual.TriggerMonitor$EventState, value: java.util.LinkedList #278
     <- item     - class: java.util.LinkedList$Node, value: com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.actions.manual.TriggerMonitor$EventState #1
      <- last     - class: java.util.LinkedList, value: java.util.LinkedList$Node #1236
       <- events     - class: com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.actions.manual.TriggerMonitor, value: java.util.LinkedList #276
        <- value     - class: java.util.HashMap$Entry, value: com.sonyericsson.hudson.plugins.gerrit.trigger.hudsontrigger.actions.manual.TriggerMonitor #1
         <- [0]     - class: java.util.HashMap$Entry[], value: java.util.HashMap$Entry #71453
          <- table     - class: java.util.HashMap, value: java.util.HashMap$Entry[] #22502
           <- _values     - class: org.mortbay.jetty.servlet.HashSessionManager$Session, value: java.util.HashMap #24894
            <- value     - class: java.util.HashMap$Entry, value: org.mortbay.jetty.servlet.HashSessionManager$Session #1
             <- [15]     - class: java.util.HashMap$Entry[], value: java.util.HashMap$Entry #60508
              <- table     - class: java.util.HashMap, value: java.util.HashMap$Entry[] #19294
               <- _sessions     - class: org.mortbay.jetty.servlet.HashSessionManager, value: java.util.HashMap #16704
                <- this$0 (Java frame)     - class: org.mortbay.jetty.servlet.HashSessionManager$SessionScavenger, value: org.mortbay.jetty.servlet.HashSessionManager #1
Merge branch 'reload-JENKINS-23152' of github.com:jglick/gerrit-trigg…
…er-plugin into JENKINS-23152

Conflicts:
	src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTrigger.java
	src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/spec/SpecGerritTriggerHudsonTest.java
Revert "CheckStyle is complaining that GerritTrigger.java may be 2000…
… lines long, but not 2019?!"

This reverts commit cfaea0f.
Merge remote-tracking branch 'origin/master' into JENKINS-23152
Conflicts:
	src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/model/BuildMemory.java
	src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTrigger.java
	src/test/java/com/sonyericsson/hudson/plugins/gerrit/trigger/spec/SpecGerritTriggerHudsonTest.java

Tests compiles but still fails.
Fixing tests
Fixed: GerritTriggerTest, SpecGerritVerifiedSetterTest
And some issues those tests found.

Tests still failing:
  BackCompat2101HudsonTest, MemoryImprintTest, ToGerritRunListenerTest
@rsandell

This comment has been minimized.

Copy link
Member Author

commented Dec 15, 2014

Most of it is done, just a few tweaks here and there, but I'm starting the pull request early since there is a lot to go through for anyone reviewing.

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Dec 15, 2014

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

@hugares

This comment has been minimized.

Copy link
Member

commented Dec 15, 2014

I reviewed the change quickly and I do not see any major problems. There are 14 failing tests on my side, it could be my env that is broken...

Bumped Jenkins dependency to 1.565.3
To get closer to current core behaviour for further testing.
*/
@CheckForNull
private GerritTrigger getTrigger() {
AbstractProject p = Jenkins.getInstance().getItemByFullName(job, AbstractProject.class);

This comment has been minimized.

Copy link
@rsandell

rsandell Dec 19, 2014

Author Member

One known issue here is that it won't handle renames well.
A rename from the UI config page should be fine, but a reload from disk or item move could still cause old event listeners to be left in the list and potentially lose its corresponding Job that it should trigger.

So I'm thinking about making some kind of ItemReference class with an ItemListener somewhere that can update all the references somehow for the next step. But that could probably be in another change than this one.

This comment has been minimized.

Copy link
@rinrinne

rinrinne Dec 19, 2014

Member

I prefer to move ItemReference related things to core. Because all plugins which includes trigger feature might have similar issue...

This comment has been minimized.

Copy link
@rsandell

rsandell Dec 19, 2014

Author Member

Yea, but then we would have to wait forever for it to come into an LTS version that we can depend on.
Maybe make a separate library plugin with that feature instead?

Two more duplication tests
CLI and Http configure.
Also activated the old duplication tests again, miht have been missed in the jenkins rule conversion.
Fixed a flaky test.

rsandell added a commit that referenced this pull request Dec 22, 2014

@rsandell rsandell merged commit c3899ea into master Dec 22, 2014

1 of 2 checks passed

default Jenkins is validating pull request ...
Details
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.