Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-56252

Futures.addCallback uses method deleted in Guava 21

    Details

    • Similar Issues:

      Description

      If you update to Guava 21+ and try to run a Pipeline build you will get

      java.lang.NoSuchMethodError: com.google.common.util.concurrent.MoreExecutors.sameThreadExecutor()Lcom/google/common/util/concurrent/ListeningExecutorService;
      	at org.jenkinsci.plugins.workflow.support.concurrent.Futures.addCallback(Futures.java:90)
      	at org.jenkinsci.plugins.workflow.cps.CpsFlowExecution.runInCpsVmThread(CpsFlowExecution.java:738)
      	at ...
      

      due to this removal. (Note that the method was not deprecated or even marked @Beta in Guava 11, which we currently compile against—the deprecation happened later.)

        Attachments

          Activity

          Hide
          basil Basil Crow added a comment -

          Hey Jesse Glick, I started working on this over the weekend and achieved some preliminary success converting workflow-support to work with both Guava 11 and Guava 28.1. You can see my work-in-progress changes here.

          While running some integration tests with the above, I found three remaining odds and ends.

          In workflow-basic-steps's TimeoutStepExecution we directly call MoreExecutors.sameThreadExecutor, which was removed in Guava 21:

          workflow-basic-steps-plugin/src/main/java/org/jenkinsci/plugins/workflow/steps/TimeoutStepExecution.java
          import com.google.common.util.concurrent.MoreExecutors;
          […]
          // TODO would use Futures.addCallback but this is still @Beta in Guava 19 and the Pipeline copy is in workflow-support on which we have no dep
          currentExecutions.addListener(new Runnable() {
              @Override public void run() {
                  […]
              } 
          }, MoreExecutors.sameThreadExecutor());
          

          In workflow-api's FlowExecutionList we call com.google.common.util.concurrent.Futures#addCallback(ListenableFuture, FutureCallback) directly (as opposed to org.jenkinsci.plugins.workflow.support.concurrent.Futures#addCallback(ListenableFuture, FutureCallback)):

          workflow-api-plugin/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionList.java
          import com.google.common.util.concurrent.Futures;
          […]
          Futures.addCallback(e.getCurrentExecutions(false), new FutureCallback<List<StepExecution>>() {
              […]
          });
          

          com.google.common.util.concurrent.Futures#addCallback(ListenableFuture, FutureCallback) was removed in Guava 26, yielding errors like this even with my workflow-support fixes:

          java.lang.NoSuchMethodError: com.google.common.util.concurrent.Futures.addCallback(Lcom/google/common/util/concurrent/ListenableFuture;Lcom/google/common/util/concurrent/FutureCallback;)V
                  at org.jenkinsci.plugins.workflow.flow.FlowExecutionList$ItemListenerImpl.onLoaded(FlowExecutionList.java:180)
                  at jenkins.model.Jenkins.<init>(Jenkins.java:989)
                  at hudson.model.Hudson.<init>(Hudson.java:85)
          

          Finally, workflow-step-api's StepExecution uses Futures#allAsList:

          workflow-step-api-plugin/src/main/java/org/jenkinsci/plugins/workflow/steps/StepExecution.java
          import com.google.common.util.concurrent.Futures;
          […]
          return Futures.allAsList(futures);
          

          This hasn't changed in the latest version of Guava, but it makes me nervous that we are using com.google.common.util.concurrent.Futures anywhere in Pipeline rather than the safer org.jenkinsci.plugins.workflow.support.concurrent.Futures.

          What is to be done for these three cases? workflow-api depends on workflow-step-api and workflow-support depends on workflow-api. So one solution would be to move the org.jenkinsci.plugins.workflow.support.concurrent package out of workflow-support. But where should it go? Since it is an internal API for pipeline, I think a logical place would be a org.jenkinsci.plugins.workflow.concurrent package in workflow-api. But then workflow-step-api wouldn't have access to it. Putting the org.jenkinsci.plugins.workflow.concurrent package in workflow-step-api doesn't seem like a good place either, since it isn't clearly related to the Step API. And making a whole new plugin for this seems overkill. So I can't think of a good solution here. Maybe we should put it in workflow-api and copy and paste a private restricted copy in workflow-step-api? That isn't the cleanest solution, but it's the best compromise I can think of right now.

          Here's my proposed plan of action. Let me know what you think.

          1. Open PR ① against workflow-api to copy the org.jenkinsci.plugins.workflow.support.concurrent package from workflow-support into a org.jenkinsci.plugins.workflow.concurrent package in workflow-api.
          2. Open PR ② against workflow-api (downstream of PR ①) with the changes from here to make org.jenkinsci.plugins.workflow.concurrent.Futures work with newer Guava versions.
          3. Open PR ③ against workflow-step-api (downstream of PR ②) to make a restricted copy of the needed org.jenkinsci.plugins.workflow.concurrent classes from PR ② and consume them in StepExecution.
          4. Open PR ④ against workflow-support (downstream of PRs ② and ③) to replace usages of org.jenkinsci.plugins.workflow.support.concurrent with usages of org.jenkinsci.plugins.workflow.concurrent and deprecate all
            org.jenkinsci.plugins.workflow.support.concurrent classes.
          5. Open PR ⑤ against workflow-cps (downstream of PRs ②, ③, and ④) to replace usages of org.jenkinsci.plugins.workflow.support.concurrent with usages of org.jenkinsci.plugins.workflow.concurrent and to update CpsThreadGroup to use org.jenkinsci.plugins.workflow.concurrent.Futures rather than com.google.common.util.concurrent.Futures.
          6. Open PRs ⑥, ⑦, and ⑧ against workflow-basic-steps, workflow-durable-task, and workflow-job (downstream of PRs ②, ③, ④, and ⑤) to replace usages of org.jenkinsci.plugins.workflow.support.concurrent with usages of org.jenkinsci.plugins.workflow.concurrent and to start using org.jenkinsci.plugins.workflow.concurrent.Futures#addCallback in TimeoutStepExecution rather than the current code referenced above which uses MoreExecutors directly.
          7. Open "do not merge" PRs ⑨, ⑩, ⑪, ⑫, ⑬, ⑭, and ⑮ against workflow-api, workflow-step-api, workflow-support, workflow-cps, workflow-basic-steps, workflow-durable-task, and workflow-job (downstream of the preceding PRs above) with a Guava update to Guava 28.1 to demonstrate that all preceding work results in Pipeline being compatible with the newest version of Guava. The preceding PRs would have demonstrated that it still works with Guava 11.

          This is a formidable amount of work, so before I get started I wanted to get your feedback.

          Show
          basil Basil Crow added a comment - Hey Jesse Glick , I started working on this over the weekend and achieved some preliminary success converting workflow-support to work with both Guava 11 and Guava 28.1. You can see my work-in-progress changes here . While running some integration tests with the above, I found three remaining odds and ends. In workflow-basic-steps 's TimeoutStepExecution we directly call MoreExecutors.sameThreadExecutor , which was removed in Guava 21: workflow-basic-steps-plugin/src/main/java/org/jenkinsci/plugins/workflow/steps/TimeoutStepExecution.java import com.google.common.util.concurrent.MoreExecutors; […] // TODO would use Futures.addCallback but this is still @Beta in Guava 19 and the Pipeline copy is in workflow-support on which we have no dep currentExecutions.addListener( new Runnable () { @Override public void run() { […] } }, MoreExecutors.sameThreadExecutor()); In workflow-api 's FlowExecutionList we call com.google.common.util.concurrent.Futures#addCallback(ListenableFuture, FutureCallback) directly (as opposed to org.jenkinsci.plugins.workflow.support.concurrent.Futures#addCallback(ListenableFuture, FutureCallback) ): workflow-api-plugin/src/main/java/org/jenkinsci/plugins/workflow/flow/FlowExecutionList.java import com.google.common.util.concurrent.Futures; […] Futures.addCallback(e.getCurrentExecutions( false ), new FutureCallback<List<StepExecution>>() { […] }); com.google.common.util.concurrent.Futures#addCallback(ListenableFuture, FutureCallback) was removed in Guava 26, yielding errors like this even with my workflow-support fixes: java.lang.NoSuchMethodError: com.google.common.util.concurrent.Futures.addCallback(Lcom/google/common/util/concurrent/ListenableFuture;Lcom/google/common/util/concurrent/FutureCallback;)V at org.jenkinsci.plugins.workflow.flow.FlowExecutionList$ItemListenerImpl.onLoaded(FlowExecutionList.java:180) at jenkins.model.Jenkins.<init>(Jenkins.java:989) at hudson.model.Hudson.<init>(Hudson.java:85) Finally, workflow-step-api 's StepExecution uses Futures#allAsList : workflow-step-api-plugin/src/main/java/org/jenkinsci/plugins/workflow/steps/StepExecution.java import com.google.common.util.concurrent.Futures; […] return Futures.allAsList(futures); This hasn't changed in the latest version of Guava, but it makes me nervous that we are using com.google.common.util.concurrent.Futures anywhere in Pipeline rather than the safer org.jenkinsci.plugins.workflow.support.concurrent.Futures . What is to be done for these three cases? workflow-api depends on workflow-step-api and workflow-support depends on workflow-api . So one solution would be to move the org.jenkinsci.plugins.workflow.support.concurrent package out of workflow-support . But where should it go? Since it is an internal API for pipeline, I think a logical place would be a org.jenkinsci.plugins.workflow.concurrent package in workflow-api . But then workflow-step-api wouldn't have access to it. Putting the org.jenkinsci.plugins.workflow.concurrent package in workflow-step-api doesn't seem like a good place either, since it isn't clearly related to the Step API. And making a whole new plugin for this seems overkill. So I can't think of a good solution here. Maybe we should put it in workflow-api and copy and paste a private restricted copy in workflow-step-api ? That isn't the cleanest solution, but it's the best compromise I can think of right now. Here's my proposed plan of action. Let me know what you think. Open PR ① against workflow-api to copy the org.jenkinsci.plugins.workflow.support.concurrent package from workflow-support into a org.jenkinsci.plugins.workflow.concurrent package in workflow-api . Open PR ② against workflow-api (downstream of PR ①) with the changes from here to make org.jenkinsci.plugins.workflow.concurrent.Futures work with newer Guava versions. Open PR ③ against workflow-step-api (downstream of PR ②) to make a restricted copy of the needed org.jenkinsci.plugins.workflow.concurrent classes from PR ② and consume them in StepExecution . Open PR ④ against workflow-support (downstream of PRs ② and ③) to replace usages of org.jenkinsci.plugins.workflow.support.concurrent with usages of org.jenkinsci.plugins.workflow.concurrent and deprecate all org.jenkinsci.plugins.workflow.support.concurrent classes. Open PR ⑤ against workflow-cps (downstream of PRs ②, ③, and ④) to replace usages of org.jenkinsci.plugins.workflow.support.concurrent with usages of org.jenkinsci.plugins.workflow.concurrent and to update CpsThreadGroup to use org.jenkinsci.plugins.workflow.concurrent.Futures rather than com.google.common.util.concurrent.Futures . Open PRs ⑥, ⑦, and ⑧ against workflow-basic-steps , workflow-durable-task , and workflow-job (downstream of PRs ②, ③, ④, and ⑤) to replace usages of org.jenkinsci.plugins.workflow.support.concurrent with usages of org.jenkinsci.plugins.workflow.concurrent and to start using org.jenkinsci.plugins.workflow.concurrent.Futures#addCallback in TimeoutStepExecution rather than the current code referenced above which uses MoreExecutors directly. Open "do not merge" PRs ⑨, ⑩, ⑪, ⑫, ⑬, ⑭, and ⑮ against workflow-api , workflow-step-api , workflow-support , workflow-cps , workflow-basic-steps , workflow-durable-task , and workflow-job (downstream of the preceding PRs above) with a Guava update to Guava 28.1 to demonstrate that all preceding work results in Pipeline being compatible with the newest version of Guava. The preceding PRs would have demonstrated that it still works with Guava 11. This is a formidable amount of work, so before I get started I wanted to get your feedback.
          Hide
          basil Basil Crow added a comment -

          After writing the above, I started wondering whether it would be possible to bundle the latest version of Guava with Pipeline and have Pipeline use that instead of the Guava that is bundled with Jenkins core. I'm not sure offhand if this is possible and I haven't done any experiments yet. If possible, maybe we could follow an alternative plan along these lines:

          1. Open PR ① against workflow-step-api to update the Guava dependency to 28.1 in dependencyManagement.
          2. Open PR ② against workflow-api (downstream of PR ①) to update the Guava dependency to 28.1 in dependencyManagement.
          3. Open PR ③ against workflow-support (downstream of PRs ① and ②) to deprecate all org.jenkinsci.plugins.workflow.support.concurrent classes, update the Guava dependency to 28.1 in dependencyManagement, and replace any usages of org.jenkinsci.plugins.workflow.support.concurrent with com.google.common.util.concurrent.
          4. Open PR ④ against workflow-cps (downstream of PRs ①, ②, and ③) to update the Guava dependency to 28.1 in dependencyManagement and replace any usages of org.jenkinsci.plugins.workflow.support.concurrent with com.google.common.util.concurrent.
          5. Open PRs ⑤, ⑥, and ⑦ against workflow-basic-steps, workflow-durable-task, and workflow-job (downstream of PRs ①, ②, ③, ④ as applicable) to update the Guava dependency to 28.1 in dependencyManagement, replace any usages of org.jenkinsci.plugins.workflow.support.concurrent with com.google.common.util.concurrent, and to start using com.google.common.util.concurrent.Futures#addCallback in TimeoutStepExecution rather than the current code referenced above which uses MoreExecutors directly.
          Show
          basil Basil Crow added a comment - After writing the above, I started wondering whether it would be possible to bundle the latest version of Guava with Pipeline and have Pipeline use that instead of the Guava that is bundled with Jenkins core. I'm not sure offhand if this is possible and I haven't done any experiments yet. If possible, maybe we could follow an alternative plan along these lines: Open PR ① against workflow-step-api to update the Guava dependency to 28.1 in dependencyManagement . Open PR ② against workflow-api (downstream of PR ①) to update the Guava dependency to 28.1 in dependencyManagement . Open PR ③ against workflow-support (downstream of PRs ① and ②) to deprecate all org.jenkinsci.plugins.workflow.support.concurrent classes, update the Guava dependency to 28.1 in dependencyManagement , and replace any usages of org.jenkinsci.plugins.workflow.support.concurrent with com.google.common.util.concurrent . Open PR ④ against workflow-cps (downstream of PRs ①, ②, and ③) to update the Guava dependency to 28.1 in dependencyManagement and replace any usages of org.jenkinsci.plugins.workflow.support.concurrent with com.google.common.util.concurrent . Open PRs ⑤, ⑥, and ⑦ against workflow-basic-steps , workflow-durable-task , and workflow-job (downstream of PRs ①, ②, ③, ④ as applicable) to update the Guava dependency to 28.1 in dependencyManagement , replace any usages of org.jenkinsci.plugins.workflow.support.concurrent with com.google.common.util.concurrent , and to start using com.google.common.util.concurrent.Futures#addCallback in TimeoutStepExecution rather than the current code referenced above which uses MoreExecutors directly.
          Hide
          basil Basil Crow added a comment -

          After writing the above, I started wondering whether it would be possible to bundle the latest version of Guava with Pipeline and have Pipeline use that instead of the Guava that is bundled with Jenkins core.

          After reading JENKINS-36779 I think I see why this is not possible: because Pipeline calls into Jenkins core, and Jenkins core needs the old version of Guava. So I think the plan from my second comment is not viable, but the plan from my first comment still seems viable.

          Show
          basil Basil Crow added a comment - After writing the above, I started wondering whether it would be possible to bundle the latest version of Guava with Pipeline and have Pipeline use that instead of the Guava that is bundled with Jenkins core. After reading JENKINS-36779 I think I see why this is not possible: because Pipeline calls into Jenkins core, and Jenkins core needs the old version of Guava. So I think the plan from my second comment is not viable, but the plan from my first comment still seems viable.
          Hide
          jglick Jesse Glick added a comment -

          We could only bundle a newer Guava by shading it, which is possible but

          • would increase download size quite a bit (unless we set a build-time option to strip unused methods/classes)
          • makes life harder if we ever need to ship, say, an emergency security update

          So if possible I would prefer to use the version from core.

          I think it is OK to put shared utilities into workflow-step-api if they are @Restricted(Beta.class) and clearly documented as being solely for the use of other workflow-* plugins.

          Devin Nusbaum as maintainer (last I checked) should weigh in here.

          Show
          jglick Jesse Glick added a comment - We could only bundle a newer Guava by shading it, which is possible but would increase download size quite a bit (unless we set a build-time option to strip unused methods/classes) makes life harder if we ever need to ship, say, an emergency security update So if possible I would prefer to use the version from core. I think it is OK to put shared utilities into workflow-step-api if they are @Restricted(Beta.class) and clearly documented as being solely for the use of other workflow-* plugins. Devin Nusbaum as maintainer (last I checked) should weigh in here.
          Hide
          basil Basil Crow added a comment -

          I think it is OK to put shared utilities into workflow-step-api if they are @Restricted(Beta.class) and clearly documented as being solely for the use of other workflow-* plugins.

          Sounds like Jesse Glick and I are in agreement so far that we should follow the plan roughly outlined by my first comment, modulo putting the org.jenkinsci.plugins.workflow.concurrent package in workflow-step-api rather than workflow-api.

          At this point I just need confirmation from Devin Nusbaum about this plan of action. Then I will begin submitting PRs.

          Show
          basil Basil Crow added a comment - I think it is OK to put shared utilities into workflow-step-api if they are @Restricted(Beta.class) and clearly documented as being solely for the use of other workflow-* plugins. Sounds like Jesse Glick and I are in agreement so far that we should follow the plan roughly outlined by my first comment, modulo putting the org.jenkinsci.plugins.workflow.concurrent package in workflow-step-api rather than workflow-api . At this point I just need confirmation from Devin Nusbaum about this plan of action. Then I will begin submitting PRs.
          Hide
          dnusbaum Devin Nusbaum added a comment -

          That plan seems fine to me, but I'm not sure I really understand the motivation, since I agree with Jesse that we should keep using the version from core rather than trying to bundle a newer version. I guess it's just nice to have it working on both versions of Guava in case all of the problems caused by the Guava update in other plugins get fixed eventually and core updates? Not sure how likely that is to ever happen, but seems fine to try to improve the situation here if you want to work on it.

          Show
          dnusbaum Devin Nusbaum added a comment - That plan seems fine to me, but I'm not sure I really understand the motivation, since I agree with Jesse that we should keep using the version from core rather than trying to bundle a newer version. I guess it's just nice to have it working on both versions of Guava in case all of the problems caused by the Guava update in other plugins get fixed eventually and core updates? Not sure how likely that is to ever happen, but seems fine to try to improve the situation here if you want to work on it.
          Hide
          basil Basil Crow added a comment -

          Thanks for the reply!

          I guess it's just nice to have it working on both versions of Guava in case all of the problems caused by the Guava update in other plugins get fixed eventually and core updates?

          Yes, exactly.

          Not sure how likely that is to ever happen

          I'm a fighter, not a quitter!

          Show
          basil Basil Crow added a comment - Thanks for the reply! I guess it's just nice to have it working on both versions of Guava in case all of the problems caused by the Guava update in other plugins get fixed eventually and core updates? Yes, exactly. Not sure how likely that is to ever happen I'm a fighter, not a quitter!
          Hide
          jglick Jesse Glick added a comment -

          I guess it's just nice to have it working on both versions of Guava in case all of the problems caused by the Guava update in other plugins get fixed eventually and core updates?

          I ran into this just trying to run integration tests on a plugin using a newer Guava legitimately (pluginFirstClassLoader). Of course JENKINS-41827 is the real issue.

          Show
          jglick Jesse Glick added a comment - I guess it's just nice to have it working on both versions of Guava in case all of the problems caused by the Guava update in other plugins get fixed eventually and core updates? I ran into this just trying to run integration tests on a plugin using a newer Guava legitimately ( pluginFirstClassLoader ). Of course JENKINS-41827 is the real issue.

            People

            • Assignee:
              Unassigned
              Reporter:
              jglick Jesse Glick
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated: