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.

Sign up for GitHub

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

Make workflow-support compatible with Guava 21.0 and newer #114

Merged
merged 11 commits into from
Mar 4, 2021
Merged

Make workflow-support compatible with Guava 21.0 and newer #114

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6ba5df6
Upgrade guava
timja Jan 16, 2021
335977a
Merge branch 'master' into test-new-guava
timja Feb 4, 2021
3a32092
Update pom.xml
timja Feb 4, 2021
b416720
Merge branch 'master' into test-new-guava
timja Feb 6, 2021
902967c
Merge branch 'test-new-guava' of github.com:timja/workflow-support-pl…
timja Feb 6, 2021
b0ce2e0
Use reflection to fallback to older guava API and avoid core bump
amuniz Mar 3, 2021
f7a4a16
Javadocs
amuniz Mar 3, 2021
aa4aba8
Merge pull request #1 from amuniz/test-new-guava
timja Mar 3, 2021
2ff560a
Remove unneeded exclusion
timja Mar 3, 2021
f274d5d
Apply suggestions from code review
timja Mar 3, 2021
c5ecc11
Invert guava method
timja Mar 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .mvn/extensions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@
<extension>
<groupId>io.jenkins.tools.incrementals</groupId>
<artifactId>git-changelist-maven-extension</artifactId>
<version>1.0-beta-4</version>
<version>1.2</version>
</extension>
</extensions>
15 changes: 11 additions & 4 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@
</license>
</licenses>
<scm>
<connection>scm:git:git://github.com/jenkinsci/${project.artifactId}-plugin.git</connection>
<developerConnection>scm:git:git@github.com:jenkinsci/${project.artifactId}-plugin.git</developerConnection>
<url>https://github.com/jenkinsci/${project.artifactId}-plugin</url>
<connection>scm:git:git://github.com/${gitHubRepo}.git</connection>
<developerConnection>scm:git:git@github.com:${gitHubRepo}.git</developerConnection>
<url>https://github.com/${gitHubRepo}</url>
<tag>${scmTag}</tag>
</scm>
<repositories>
Expand All @@ -64,11 +64,12 @@
<properties>
<revision>3.8</revision>
<changelist>-SNAPSHOT</changelist>
<jenkins.version>2.222.4</jenkins.version>
<jenkins.version>2.273-rc30689.09147672b85b</jenkins.version>
Copy link
Member

Choose a reason for hiding this comment

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

Can you please link to whatever Jenkins core PR produced this incremental so reviewers have more context about the proposed change? Thanks!

timja marked this conversation as resolved.
Show resolved Hide resolved
<java.level>8</java.level>
<no-test-jar>false</no-test-jar>
<useBeta>true</useBeta>
<hpi.compatibleSinceVersion>3.0</hpi.compatibleSinceVersion>
<gitHubRepo>jenkinsci/${project.artifactId}-plugin</gitHubRepo>
</properties>
<dependencyManagement>
<dependencies>
Expand Down Expand Up @@ -97,6 +98,12 @@
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>script-security</artifactId>
<exclusions>
<exclusion>
<groupId>org.checkerframework</groupId>
<artifactId>checker-qual</artifactId>
</exclusion>
</exclusions>
timja marked this conversation as resolved.
Show resolved Hide resolved
</dependency>
<dependency>
<!-- Required for running with Java 9+ -->
Expand Down
2 changes: 1 addition & 1 deletion ...main/java/org/jenkinsci/plugins/workflow/support/concurrent/ChainingListenableFuture.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ public void run() {
ChainingListenableFuture.this.outputFuture = null;
}
}
}, MoreExecutors.sameThreadExecutor());
}, MoreExecutors.newDirectExecutorService());
} catch (UndeclaredThrowableException e) {
// Set the cause of the exception as this future's exception
setException(e.getCause());
Expand Down
28 changes: 14 additions & 14 deletions src/main/java/org/jenkinsci/plugins/workflow/support/concurrent/Futures.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public abstract class Futures {
* });}</pre>
*
* <p>Note: This overload of {@code addCallback} is designed for cases in
* which the callack is fast and lightweight, as the method does not accept
* which the callback is fast and lightweight, as the method does not accept
* an {@code Executor} in which to perform the the work. For heavier
* callbacks, this overload carries some caveats: First, the thread that the
* callback runs in depends on whether the input {@code Future} is done at the
Expand All @@ -74,7 +74,7 @@ public abstract class Futures {
* callback in the thread that calls {@code addCallback} or {@code
* Future.cancel}. Second, callbacks may run in an internal thread of the
* system responsible for the input {@code Future}, such as an RPC network
* thread. Finally, during the execution of a {@code sameThreadExecutor}
* thread. Finally, during the execution of a {@code newDirectExecutorService}
* callback, all other registered but unexecuted listeners are prevented from
* running, even if those listeners are to run in other executors.
*
Expand All @@ -87,7 +87,7 @@ public abstract class Futures {
*/
public static <V> void addCallback(ListenableFuture<V> future,
FutureCallback<? super V> callback) {
addCallback(future, callback, MoreExecutors.sameThreadExecutor());
addCallback(future, callback, MoreExecutors.newDirectExecutorService());
}

/**
Expand Down Expand Up @@ -115,16 +115,16 @@ public static <V> void addCallback(ListenableFuture<V> future,
*
* When the callback is fast and lightweight consider {@linkplain
* Futures#addCallback(ListenableFuture, FutureCallback) the other overload}
* or explicit use of {@link MoreExecutors#sameThreadExecutor
* sameThreadExecutor}. For heavier callbacks, this choice carries some
* or explicit use of {@link MoreExecutors#newDirectExecutorService
* newDirectExecutorService}. For heavier callbacks, this choice carries some
* caveats: First, the thread that the callback runs in depends on whether
* the input {@code Future} is done at the time {@code addCallback} is called
* and on whether the input {@code Future} is ever cancelled. In particular,
* {@code addCallback} may execute the callback in the thread that calls
* {@code addCallback} or {@code Future.cancel}. Second, callbacks may run in
* an internal thread of the system responsible for the input {@code Future},
* such as an RPC network thread. Finally, during the execution of a {@code
* sameThreadExecutor} callback, all other registered but unexecuted
* newDirectExecutorService} callback, all other registered but unexecuted
* listeners are prevented from running, even if those listeners are to run
* in other executors.
*
Expand Down Expand Up @@ -218,7 +218,7 @@ public static <V> ListenableFuture<V> immediateFailedFuture(
* thread that called {@code transform}. Second, transformations may run in
* an internal thread of the system responsible for the input {@code Future},
* such as an RPC network thread. Finally, during the execution of a {@code
* sameThreadExecutor} transformation, all other registered but unexecuted
* newDirectExecutorService} transformation, all other registered but unexecuted
* listeners are prevented from running, even if those listeners are to run
* in other executors.
*
Expand All @@ -240,7 +240,7 @@ public static <V> ListenableFuture<V> immediateFailedFuture(
*/
public static <I, O> ListenableFuture<O> transform(ListenableFuture<I> future,
final Function<? super I, ? extends O> function) {
return Futures.<I, O>transform(future, function, MoreExecutors.sameThreadExecutor());
return Futures.<I, O>transform(future, function, MoreExecutors.newDirectExecutorService());
}

/**
Expand Down Expand Up @@ -272,15 +272,15 @@ public static <I, O> ListenableFuture<O> transform(ListenableFuture<I> future,
* <p>Note: For cases in which the transformation is fast and lightweight,
* consider {@linkplain Futures#transform(ListenableFuture, Function) the
* other overload} or explicit use of {@link
* MoreExecutors#sameThreadExecutor}. For heavier transformations, this
* MoreExecutors#newDirectExecutorService}. For heavier transformations, this
* choice carries some caveats: First, the thread that the transformation
* runs in depends on whether the input {@code Future} is done at the time
* {@code transform} is called. In particular, if called late, {@code
* transform} will perform the transformation in the thread that called
* {@code transform}. Second, transformations may run in an internal thread
* of the system responsible for the input {@code Future}, such as an RPC
* network thread. Finally, during the execution of a {@code
* sameThreadExecutor} transformation, all other registered but unexecuted
* newDirectExecutorService} transformation, all other registered but unexecuted
* listeners are prevented from running, even if those listeners are to run
* in other executors.
*
Expand Down Expand Up @@ -324,7 +324,7 @@ public static <I, O> ListenableFuture<O> transform(ListenableFuture<I> future,
public static <V> ListenableFuture<List<V>> allAsList(
ListenableFuture<? extends V>... futures) {
return new ListFuture<V>(ImmutableList.copyOf(futures), true,
MoreExecutors.sameThreadExecutor());
MoreExecutors.newDirectExecutorService());
Copy link
Member

Choose a reason for hiding this comment

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

Since this is since Guava 18, maybe we could use reflection to try newDirectExecutorService then fall back to sameThreadExecutor, then revert the core bump?

Or copy the impl into this plugin, keeping the Apache license header.

Copy link
Member Author

Choose a reason for hiding this comment

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

nice idea, when I was working on it (and plan to get back to it, although others are welcome to take it up) I was trying to get a green bom PCT build and then look at what's required and see where changes like your suggestion could be applied.

Copy link
Member

Choose a reason for hiding this comment

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

I have this change ready to push. Is it ok if I push to your branch @timja?

Copy link
Member Author

Choose a reason for hiding this comment

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

absolutely go for it

Copy link
Member Author

Choose a reason for hiding this comment

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

also see jenkinsci/workflow-api-plugin#135 and jenkinsci/bom#423

Copy link
Member

Choose a reason for hiding this comment

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

Errr, I don't have push permission here, so I've filed a PR to your forked branch: timja#1

Copy link
Member

Choose a reason for hiding this comment

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

also see jenkinsci/workflow-api-plugin#135 and jenkinsci/bom#423

I'll have a look. Meanwhile, isn't this PR mergeable as is?

Copy link
Member Author

Choose a reason for hiding this comment

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

looks good to me

}

/**
Expand All @@ -347,7 +347,7 @@ public static <V> ListenableFuture<List<V>> allAsList(
public static <V> ListenableFuture<List<V>> allAsList(
Iterable<? extends ListenableFuture<? extends V>> futures) {
return new ListFuture<V>(ImmutableList.copyOf(futures), true,
MoreExecutors.sameThreadExecutor());
MoreExecutors.newDirectExecutorService());
}

/**
Expand Down Expand Up @@ -379,14 +379,14 @@ public static <V> ListenableFuture<List<V>> allAsList(
* <p>Note: For cases in which the work of creating the derived future is
* fast and lightweight, consider {@linkplain Futures#chain(ListenableFuture,
* Function) the other overload} or explicit use of {@code
* sameThreadExecutor}. For heavier derivations, this choice carries some
* newDirectExecutorService}. For heavier derivations, this choice carries some
* caveats: First, the thread that the derivation runs in depends on whether
* the input {@code Future} is done at the time {@code chain} is called. In
* particular, if called late, {@code chain} will run the derivation in the
* thread that called {@code chain}. Second, derivations may run in an
* internal thread of the system responsible for the input {@code Future},
* such as an RPC network thread. Finally, during the execution of a {@code
* sameThreadExecutor} {@code chain} function, all other registered but
* newDirectExecutorService} {@code chain} function, all other registered but
* unexecuted listeners are prevented from running, even if those listeners
* are to run in other executors.
*
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/jenkinsci/plugins/workflow/support/concurrent/ListFuture.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public void run() {
// Let go of the memory held by other futures
ListFuture.this.futures = null;
}
}, MoreExecutors.sameThreadExecutor());
}, MoreExecutors.newDirectExecutorService());

// Now begin the "real" initialization.

Expand Down