[JENKINS-48729] FindBugs reports concurrency issues in Lib Task Reactor

[timja]

Task Reactor is a mission-critical component of the Jenkins initialization logic. IMHO we need to investigate the issues and fix/suppress them

[INFO] --- findbugs-maven-plugin:3.0.5:check (default) @ task-reactor ---
[INFO] BugInstance size is 3
[INFO] Error size is 0
[INFO] Total bugs: 3
[INFO] Inconsistent synchronization of org.jvnet.hudson.reactor.Reactor.executor; locked 50% of time [org.jvnet.hudson.reactor.Reactor$Node, org.jvnet.hudson.reactor.Reactor$Node, org.jvnet.hudson.reactor.Reactor, org.jvnet.hudson.reactor.Reactor, org.jvnet.hudson.reactor.Reactor] Unsynchronized access at Reactor.java:[line 139]Unsynchronized access at Reactor.java:[line 109]Synchronized access at Reactor.java:[line 256]Synchronized access at Reactor.java:[line 273]Synchronized access at Reactor.java:[line 273] IS2_INCONSISTENT_SYNC
[INFO] Inconsistent synchronization of org.jvnet.hudson.reactor.Reactor.fatal; locked 75% of time [org.jvnet.hudson.reactor.Reactor$Node, org.jvnet.hudson.reactor.Reactor$Node, org.jvnet.hudson.reactor.Reactor, org.jvnet.hudson.reactor.Reactor] Unsynchronized access at Reactor.java:[line 119]Synchronized access at Reactor.java:[line 126]Synchronized access at Reactor.java:[line 268]Synchronized access at Reactor.java:[line 269] IS2_INCONSISTENT_SYNC
[INFO] Using notify rather than notifyAll in org.jvnet.hudson.reactor.Reactor$Node.run() [org.jvnet.hudson.reactor.Reactor$Node] At Reactor.java:[line 131] NO_NOTIFY_NOT_NOTIFYALL
[INFO] 

---

Originally reported by oleg_nenashev, imported from: FindBugs reports concurrency issues in Lib Task Reactor

• status: Open
• priority: Minor
• resolution: Unresolved
• imported: 2022/01/10

[timja]

oleg_nenashev:

Analysis from greybird in https://github.com/jenkinsci/envinject-api-plugin/pull/5:

• fatal:
  • the 1 out of 4 occurences where it is not synchronized is when the task sets the field after an exception.
  • as only a single exception is kept, and variable assignment is thread-safe, not a real issue
  • (also synchronization is not really related to the fatal assignment on the locations where it is present)
    executor
• not synchronized read are from task's runIfPossible (and canRun, which is only called from runIfPossible)
  • executor values are set from execute method, which is synchronized
  • runIfPossible methods are called from
    • addAll and execute, which are synchronized
    • Node.run, under a synchronize block on Reactor.this
  • so again I would say not a real issue
• notify vs notifyall
  • all threads are waiting for Reactor instance lock.
  • only threads actually capable of doing something are in the list of active tasks
  • in this situation I would say that using notifyAll would lead to waking more threads than needed
  • I'm not sure of the behavior if any external component is taking a lock on the Reactor instance however ?