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

Implement InterceptingExecutorService without Guava #5565

Merged
merged 2 commits into from
Jun 19, 2021

Conversation

basil
Copy link
Member

@basil basil commented Jun 9, 2021

Jenkins core defines jenkins.util.InterceptingExecutorService, which extends com.google.common.util.concurrent.ForwardingExecutorService. This exposes a Guava API in the Jenkins public API, which is a liability. If the Guava API changed, the Jenkins API might also have to change, which could result in incompatibilities. Supporting such an API also implies that Jenkins core must expose Guava to plugins, something we would rather avoid if possible.

This PR is going after the same goal as #5558 but with a different approach. The goal is to remove exposure of Guava in the Jenkins core public API. The approach taken in this PR is to directly implement ExecutorService rather than to extend Guava's ForwardingExecutorService. Modulo compatibility concerns, this is the simplest option to eliminating the dependency.

From a compatibility perspective this change is a bit risky, as it changes the class hierarchy for these classes:

jenkins/util/InterceptingExecutorService
jenkins/security/ImpersonatingExecutorService (extends InterceptingExecutorService)
jenkins/security/SecurityContextExecutorService (extends InterceptingExecutorService)
jenkins/util/ContextResettingExecutorService (extends InterceptingExecutorService)
jenkins/util/InterceptingScheduledExecutorService (extends InterceptingExecutorService)
jenkins/security/ImpersonatingScheduledExecutorService (extends InterceptingScheduledExecutorService)

They now no longer extend ForwardingExecutorService or ForwardingObject, so any attempts to cast to those types would fail. However, I doubt any such casts exist.

Apart from that, I've ensured that all of the same public methods are implemented, so in theory any already compiled plugins invoking methods on these objects should continue to work as before. In practice, I think we'd want to do some sort of testing on this, but I'm not sure exactly how to test this. I'll start by trying to get an incremental build and run it through the BOM tests.

Proposed changelog entries

Developer: InterceptingExecutorService and its subclasses no longer extend com.google.common.util.concurrent.ForwardingExecutorService or com.google.common.collect.ForwardingObject.

Proposed upgrade guidelines

N/A

Submitter checklist

  • (If applicable) Jira issue is well described
  • Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least 2 approvals for the pull request and no outstanding requests for change
  • Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are correct
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

@basil basil added dependencies Pull requests that update a dependency file skip-changelog Should not be shown in the changelog labels Jun 9, 2021
basil added a commit to basil/bom that referenced this pull request Jun 10, 2021
@basil
Copy link
Member Author

basil commented Jun 10, 2021

Usages are as follows:

jenkins/security/ImpersonatingExecutorService

    operations-center-client
    operations-center-context
    operations-center-server

jenkins/security/ImpersonatingScheduledExecutorService

    scm-api

jenkins/security/SecurityContextExecutorService

    lenientshutdown

jenkins/util/ContextResettingExecutorService

    operations-center-context
    operations-center-server
    workflow-cps

jenkins/util/InterceptingExecutorService

    workflow-cps

From the source side, I verified that none of the open source plugins reference ForwardingExecutorService or ForwardingObject. We definitely seem to be source compatible with all the open source plugins with this PR.

We also look good on the PCT side. PCT passed for scm-api and workflow-cps with these changes. In fact, the whole BOM run for this PR was pretty good, with the only failures being related to the Digester removal (not caused by this PR) and two timeouts (probably not caused by this PR either).

Next I'll test compatibility to make sure a version of workflow-cps built against a core from before this PR still works with a core that contains this PR. By induction, this test should prove that versions of the other plugins built against a core from before this PR should also still work with a core that contains this PR.

@basil
Copy link
Member Author

basil commented Jun 10, 2021

I built a core with this change and started it with java -jar jenkins.war with remote debug options enabled. I installed the default plugin set, which included the released version of workflow-cps that was compiled against a core without this PR. I then ran a Pipeline "hello world" and the job succeeded. To convince myself that this was truly working, I connected via the remote debugger from both a Jenkins core workspace (with these changes) and a workflow-cps workspace (last released version) and set breakpoints. I hit the breakpoints from both sides, showing that the plugin (compiled before this PR) was still able to invoke the API and that core was satisfying that invocation with the new Guava-free implementation. I will paste all stack traces I collected below as proof that I performed this compatibility test. These stack traces clearly show that binary compatibility is maintained with this change, so I am ready to move forward with this PR and I am taking it out of draft status.

setupTransients:190, CpsThreadGroup (org.jenkinsci.plugins.workflow.cps)
<init>:158, CpsThreadGroup (org.jenkinsci.plugins.workflow.cps)
start:542, CpsFlowExecution (org.jenkinsci.plugins.workflow.cps)
run:337, WorkflowRun (org.jenkinsci.plugins.workflow.job)
execute:100, ResourceController (hudson.model)
run:433, Executor (hudson.model)
<clinit>:256, CpsStepContext (org.jenkinsci.plugins.workflow.cps)
invokeStep:254, DSL (org.jenkinsci.plugins.workflow.cps)
invokeMethod:193, DSL (org.jenkinsci.plugins.workflow.cps)
invokeMethod:122, CpsScript (org.jenkinsci.plugins.workflow.cps)
call:48, PogoMetaClassSite (org.codehaus.groovy.runtime.callsite)
defaultCall:48, CallSiteArray (org.codehaus.groovy.runtime.callsite)
call:113, AbstractCallSite (org.codehaus.groovy.runtime.callsite)
methodCall:20, DefaultInvoker (com.cloudbees.groovy.cps.sandbox)
methodCall:86, ContinuationGroup (com.cloudbees.groovy.cps.impl)
dispatchOrArg:113, FunctionCallBlock$ContinuationImpl (com.cloudbees.groovy.cps.impl)
fixArg:83, FunctionCallBlock$ContinuationImpl (com.cloudbees.groovy.cps.impl)
invoke:-1, GeneratedMethodAccessor71 (sun.reflect)
invoke:43, DelegatingMethodAccessorImpl (sun.reflect)
invoke:498, Method (java.lang.reflect)
receive:72, ContinuationPtr$ContinuationImpl (com.cloudbees.groovy.cps.impl)
eval:46, ClosureBlock (com.cloudbees.groovy.cps.impl)
step:83, Next (com.cloudbees.groovy.cps)
call:174, Continuable$1 (com.cloudbees.groovy.cps)
call:163, Continuable$1 (com.cloudbees.groovy.cps)
use:129, GroovyCategorySupport$ThreadCategoryInfo (org.codehaus.groovy.runtime)
use:268, GroovyCategorySupport (org.codehaus.groovy.runtime)
run0:163, Continuable (com.cloudbees.groovy.cps)
access$001:18, SandboxContinuable (org.jenkinsci.plugins.workflow.cps)
run0:51, SandboxContinuable (org.jenkinsci.plugins.workflow.cps)
runNextChunk:185, CpsThread (org.jenkinsci.plugins.workflow.cps)
run:400, CpsThreadGroup (org.jenkinsci.plugins.workflow.cps)
access$400:96, CpsThreadGroup (org.jenkinsci.plugins.workflow.cps)
call:312, CpsThreadGroup$2 (org.jenkinsci.plugins.workflow.cps)
call:276, CpsThreadGroup$2 (org.jenkinsci.plugins.workflow.cps)
call:67, CpsVmExecutorService$2 (org.jenkinsci.plugins.workflow.cps)
run:266, FutureTask (java.util.concurrent)
run:139, SingleLaneExecutorService$1 (hudson.remoting)
run:28, ContextResettingExecutorService$1 (jenkins.util)
run:68, ImpersonatingExecutorService$1 (jenkins.security)
call:511, Executors$RunnableAdapter (java.util.concurrent)
run:266, FutureTask (java.util.concurrent)
runWorker:1149, ThreadPoolExecutor (java.util.concurrent)
run:624, ThreadPoolExecutor$Worker (java.util.concurrent)
run:748, Thread (java.lang)
submit:45, InterceptingExecutorService (jenkins.util)
start:548, CpsFlowExecution (org.jenkinsci.plugins.workflow.cps)
run:337, WorkflowRun (org.jenkinsci.plugins.workflow.job)
execute:100, ResourceController (hudson.model)
run:433, Executor (hudson.model)
submit:35, InterceptingExecutorService (jenkins.util)
scheduleRun:276, CpsThreadGroup (org.jenkinsci.plugins.workflow.cps)
resume:290, CpsThread (org.jenkinsci.plugins.workflow.cps)
run:552, CpsFlowExecution$1 (org.jenkinsci.plugins.workflow.cps)
run:38, CpsVmExecutorService$1 (org.jenkinsci.plugins.workflow.cps)
call:511, Executors$RunnableAdapter (java.util.concurrent)
run:266, FutureTask (java.util.concurrent)
run:139, SingleLaneExecutorService$1 (hudson.remoting)
run:28, ContextResettingExecutorService$1 (jenkins.util)
run:68, ImpersonatingExecutorService$1 (jenkins.security)
call:511, Executors$RunnableAdapter (java.util.concurrent)
run:266, FutureTask (java.util.concurrent)
runWorker:1149, ThreadPoolExecutor (java.util.concurrent)
run:624, ThreadPoolExecutor$Worker (java.util.concurrent)
run:748, Thread (java.lang)
isShutdown:81, InterceptingExecutorService (jenkins.util)
tearDown:169, CpsVmExecutorService (org.jenkinsci.plugins.workflow.cps)
access$200:23, CpsVmExecutorService (org.jenkinsci.plugins.workflow.cps)
run:43, CpsVmExecutorService$1 (org.jenkinsci.plugins.workflow.cps)
call:511, Executors$RunnableAdapter (java.util.concurrent)
run:266, FutureTask (java.util.concurrent)
run:139, SingleLaneExecutorService$1 (hudson.remoting)
run:28, ContextResettingExecutorService$1 (jenkins.util)
run:68, ImpersonatingExecutorService$1 (jenkins.security)
call:511, Executors$RunnableAdapter (java.util.concurrent)
run:266, FutureTask (java.util.concurrent)
runWorker:1149, ThreadPoolExecutor (java.util.concurrent)
run:624, ThreadPoolExecutor$Worker (java.util.concurrent)
run:748, Thread (java.lang)
execute:70, InterceptingExecutorService (jenkins.util)
notifyNewHead:479, CpsThreadGroup (org.jenkinsci.plugins.workflow.cps)
setNewHead:157, FlowHead (org.jenkinsci.plugins.workflow.cps)
onSuccess:424, CpsStepContext$2 (org.jenkinsci.plugins.workflow.cps)
onSuccess:381, CpsStepContext$2 (org.jenkinsci.plugins.workflow.cps)
run:917, CpsFlowExecution$4$1 (org.jenkinsci.plugins.workflow.cps)
run:38, CpsVmExecutorService$1 (org.jenkinsci.plugins.workflow.cps)
call:511, Executors$RunnableAdapter (java.util.concurrent)
run:266, FutureTask (java.util.concurrent)
run:139, SingleLaneExecutorService$1 (hudson.remoting)
run:28, ContextResettingExecutorService$1 (jenkins.util)
run:68, ImpersonatingExecutorService$1 (jenkins.security)
call:511, Executors$RunnableAdapter (java.util.concurrent)
run:266, FutureTask (java.util.concurrent)
runWorker:1149, ThreadPoolExecutor (java.util.concurrent)
run:624, ThreadPoolExecutor$Worker (java.util.concurrent)
run:748, Thread (java.lang)

@basil basil marked this pull request as ready for review June 10, 2021 03:30
@jtnord jtnord self-requested a review June 10, 2021 10:47
@basil basil requested a review from dwnusbaum June 14, 2021 16:04
Copy link
Member

@jtnord jtnord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had wanted to validate the impact of this internally, but there are some other already merged changes that cause this PR to fail the testing. So give that the scm-api is still useable and should show this is good enough.

@jtnord jtnord added the developer Changes which impact plugin developers label Jun 16, 2021
@jtnord
Copy link
Member

jtnord commented Jun 16, 2021

I think we do want to release note this at a developer level. mainly because it does make it obvious if you do fall into some issue in the future with a plugin that is not in any known update center.

@jtnord jtnord added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed skip-changelog Should not be shown in the changelog labels Jun 16, 2021
@basil basil added the squash-merge-me Unclean or useless commit history, should be merged only with squash-merge label Jun 18, 2021
@res0nance res0nance merged commit 54b5436 into jenkinsci:master Jun 19, 2021
@basil basil deleted the ExecutorService branch June 19, 2021 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file developer Changes which impact plugin developers ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback squash-merge-me Unclean or useless commit history, should be merged only with squash-merge
Projects
None yet
5 participants