Fix ConcurrentModificationExceptions thrown from BaselineNullAway #2427
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Generate changelog in
|
carterkozak
reviewed
Oct 7, 2022
| private static boolean anyProjectUsesJava15(Project proj) { | ||
| return proj.getAllprojects().stream() | ||
| .anyMatch(project -> project.getTasks().withType(JavaCompile.class).stream() | ||
| .anyMatch(project -> ImmutableList.copyOf(project.getTasks().withType(JavaCompile.class)).stream() |
Contributor
There was a problem hiding this comment.
I wonder if we would be better off replacing the Collection.stream() here using something along these lines:
project.getTasks().withType(JavaCompile.class).matching(comp -> {
JavaVersion javaVersion = JavaVersion.toVersion(comp.getTargetCompatibility());
return javaVersion == JavaVersion.VERSION_15;
}).isEmpty()I suspect the TaskContainer may be a bit smarter about modification within iteration when it controls the iteration rather than returning a stream wrapping a spliterator, but it's difficult to say without a reliable reproducer.
Contributor
Author
There was a problem hiding this comment.
I tried this by testing gw classes --stacktrace on my internal failing repo, and that command:
- fails on
com.palantir.baseline:gradle-baseline-java:4.180.0 - succeeds on
com.palantir.baseline:gradle-baseline-java:4.180.0-8-ga477298 - fails on
com.palantir.baseline:gradle-baseline-java:4.180.0-9-g05114a5
So I think the ImmutableList copy approach works and the TaskCollection one doesn't. At least it gets these runs to pass.
ash211
changed the title
ash211
force-pushed
the
aash/attempt-cme-fix
branch
from
05114a5 to
a477298
Compare
Collaborator
|
Released 4.181.0 |
This was referenced Oct 7, 2022
This was referenced Oct 7, 2022
Merged
bulldozer-bot bot
pushed a commit
to palantir/witchcraft-api
that referenced
this pull request
Oct 8, 2022
###### _excavator_ is a bot for automating changes across repositories. Changes produced by the roomba/latest-baseline-oss check. # Release Notes ## 4.181.0 | Type | Description | Link | | ---- | ----------- | ---- | | Fix | Fix ConcurrentModificationExceptions thrown from BaselineNullAway | palantir/gradle-baseline#2427 | To enable or disable this check, please contact the maintainers of Excavator.
This was referenced Oct 8, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #2413
Before this PR
java.util.ConcurrentModificationExceptionthrown from this plugin:After this PR
==COMMIT_MSG==
Fix ConcurrentModificationExceptions thrown from BaselineNullAway
==COMMIT_MSG==
Possible downsides?
I think this exception indicates that the project's task list is being modified/resolved at the same time as we iterate over it in this class.
By making a copy before iterating, I think we'll reduce the CMEs being thrown, but not actually fix the underlying issue of concurrent changes happening. I'm not super familiar with Gradle internals, but think we may instead need to do something where BaselineNullAway runs in a
doLastso it doesn't interfere with project resolution.This fix was confirmed on an internal failing repro, but we don't have a test case for it as part of this PR.
By using the TaskCollection's.anyMatch()directly, we hope that it is a bit smarter about modification during iteration versus converting to a Stream and usingStream#anyMatch().