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

Task with a doLast block using a lambda is never up-to-date #5510

Closed
sayniv opened this issue May 23, 2018 · 28 comments
Closed

Task with a doLast block using a lambda is never up-to-date #5510

sayniv opened this issue May 23, 2018 · 28 comments
Assignees
Milestone

Comments

@sayniv
Copy link

@sayniv sayniv commented May 23, 2018

Expected Behavior

Gradle should allow plugins to add doLast blocks to tasks with a lambda.

Current Behavior

Using lambdas in plugin to add doLast block to tasks breaks cacheability.

Context

I am working with a plugin that adds a doLast block to tasks of type checkStyle. This plugin is written in Java and is compiled separately and used as a dependency. In the plugin we add a doLast block with something like this:

task.doLast(t -> { //do something })

I notice that checkStyle task is not cacheable due to the doLast block being defined as a Lambda.

During execution with debug turned on, I see the following log output:

_Task myCustomCheckstyle
Appending .....
Appending actionType to build cache key: com.myorg.MyCustomPlugin$$Lambda$188/376495921
...
Build cache key for task 'myCustomCheckstyle' is b869c161eb63f4507d850045047ee9aa_

The actionType is different on each invocation because the part after the $$Lambda$188/ changes with each invocation.

When I replace the lambda with an Action, like

   task.doLast(new MyCustomDoLastAction())

caching works as expected since the actionType resolves to com.myorg.MyCustomPlugin.$MyCustomDoLastAction

Steps to Reproduce (for bugs)

  1. Create a java project on which checkstyle plugin is applied
  2. Create a project to house a plugin that scans the project on which it is applied, to find checkstyle tasks and adds a doLast block to each checkstyle task it finds. Make sure the doLast block is added with a closure block and not an explicit Action implementation.
  3. Compile the plugin and apply it to the java project created in step one.
  4. Run checkstyle task with -Dorg.gradle.caching.debug=true flag.
  5. Notice the debug log with a message of the kind:
Appending actionType to build cache key: com.myorg.MyCustomPlugin$$Lambda$188/376495921
  1. Run the checkstyle task again, notice that the task is not cacheable because the the actionType corresponding to the lambda is different, and therefore the cache key is different.
  2. Now replace the closure defining the action block in the plugin with an implementation of the action. Compile the plugin again and apply to the project.
  3. Run checkstyle task twice and notice that it is not cacheable.

Your Environment

OS: macOS High Sierra
Java Version: 1.8.0_121
Gradle Version: 4.6

@wolfs

This comment has been minimized.

Copy link
Member

@wolfs wolfs commented May 24, 2018

@sayniv Thank you for reporting. I am surprised by the bug report, given that the underlying problem should have been fixed in Java 8u40 (see the JDK bug).

I guess this is something different, right? So the code of the plugin does not change (it is compiled separately) and still the class name of the lambda changes?

@oehme

This comment has been minimized.

Copy link
Member

@oehme oehme commented May 24, 2018

Independently of this bug, don't we deactivate the cache with custom actions anyway? After all they can introduce new, untracked outputs that could break caching.

@lptr

This comment has been minimized.

Copy link
Member

@lptr lptr commented May 24, 2018

There is no major difference between task actions declared via @TaskAction and doLast(). For each action we assume that inputs and outputs are properly declared (either via @Input annotations or via the runtime API). It's not an air-tight system by any means, as we simply trust that things are set up properly.

@sayniv

This comment has been minimized.

Copy link
Author

@sayniv sayniv commented May 24, 2018

Thank you the quick response folks.

@wolfs From my understanding, that JDK bug refers to the lambda number which in my case is constant as expected $$Lambda$188. It's the 9 digit number part after the "/" that is issue in my case.

And yes you are right, the code of the plugin itself isnt changing, but the when the lambda gets translated by the JVM during runtime, it gets a new type. Atleast that is my understanding of the issue.

@sayniv sayniv closed this May 25, 2018
@sayniv sayniv reopened this May 25, 2018
@tjni

This comment has been minimized.

Copy link
Contributor

@tjni tjni commented Aug 3, 2018

It looks like the JVM will add an unstable hash code to the end of the lambda class name when calling getClass().getName() on a lambda.

I've been thinking a bit about this and wanted to run this by everyone. If indeed the assumption is that inputs and outputs are properly declared, then shouldn't the action name not be part of the cache key?

@lptr

This comment has been minimized.

Copy link
Member

@lptr lptr commented Aug 6, 2018

Can you please share a reproducible test or some results we can look at?

@david-noel

This comment has been minimized.

Copy link

@david-noel david-noel commented Aug 13, 2018

I'm seeing similar behavior when using the spring boot plugin with gradle 4.9. It appears the spring boot plugin applies an action when the JavaPlugin is also present on a project.

https://github.com/spring-projects/spring-boot/blob/master/spring-boot-project/spring-boot-tools/spring-boot-gradle-plugin/src/main/java/org/springframework/boot/gradle/plugin/JavaPluginAction.java

I notice this on the compileJava task when I have org.gradle.caching.debug=true

Appending actionType to build cache key: org.springframework.boot.gradle.plugin.JavaPluginAction$$Lambda$33/67053257

Running in Jenkins, on linux with java 1.8.0_181, every build produces a different number for the 67053257 part and I get a cache miss.

The strange part is this works fine on my Mac. The number stays the same from build to build.

Even more strange is when I run the build on my machine(Docker for Mac) with the same container used by Jenkins, the number stays the same.

@wilkinsona

This comment has been minimized.

Copy link
Contributor

@wilkinsona wilkinsona commented Aug 14, 2018

Here is a sample project that will hopefully reproduce the problem, or at least a variant of it.

You can see the type name of the lambda task action change depending on the presence of --info:

Without --info:

$ GRADLE_OPTS=-verbose:class ./gradlew compileJava --no-daemon --console=plain | grep ExamplePlugin
[Loaded com.example.ExamplePlugin from file:/Users/awilkinson/.gradle/caches/jars-3/d2a9372c2f68ba34f55fb0a07a3b11d1/buildSrc.jar]
[Loaded com.example.ExamplePlugin$$Lambda$1/837790893 from com.example.ExamplePlugin]
[Loaded com.example.ExamplePlugin$$Lambda$2/1111413685 from com.example.ExamplePlugin]

Without --info again. Names are unchanged:

$ GRADLE_OPTS=-verbose:class ./gradlew compileJava --no-daemon --console=plain | grep ExamplePlugin
[Loaded com.example.ExamplePlugin from file:/Users/awilkinson/.gradle/caches/jars-3/d2a9372c2f68ba34f55fb0a07a3b11d1/buildSrc.jar]
[Loaded com.example.ExamplePlugin$$Lambda$1/837790893 from com.example.ExamplePlugin]
[Loaded com.example.ExamplePlugin$$Lambda$2/1111413685 from com.example.ExamplePlugin]

With --info. The names have changed:

$ GRADLE_OPTS=-verbose:class ./gradlew compileJava --no-daemon --console=plain --info | grep ExamplePlugin
[Loaded com.example.ExamplePlugin from file:/Users/awilkinson/.gradle/caches/jars-3/d2a9372c2f68ba34f55fb0a07a3b11d1/buildSrc.jar]
[Loaded com.example.ExamplePlugin$$Lambda$1/1938374337 from com.example.ExamplePlugin]
[Loaded com.example.ExamplePlugin$$Lambda$2/1728266914 from com.example.ExamplePlugin]

With --info again. The names have remained the same:

$ GRADLE_OPTS=-verbose:class ./gradlew compileJava --no-daemon --console=plain --info | grep ExamplePlugin
[Loaded com.example.ExamplePlugin from file:/Users/awilkinson/.gradle/caches/jars-3/d2a9372c2f68ba34f55fb0a07a3b11d1/buildSrc.jar]
[Loaded com.example.ExamplePlugin$$Lambda$1/1938374337 from com.example.ExamplePlugin]
[Loaded com.example.ExamplePlugin$$Lambda$2/1728266914 from com.example.ExamplePlugin]

Without --info. The names are back to what they were originally:

$ GRADLE_OPTS=-verbose:class ./gradlew compileJava --no-daemon --console=plain | grep ExamplePlugin
[Loaded com.example.ExamplePlugin from file:/Users/awilkinson/.gradle/caches/jars-3/d2a9372c2f68ba34f55fb0a07a3b11d1/buildSrc.jar]
[Loaded com.example.ExamplePlugin$$Lambda$1/837790893 from com.example.ExamplePlugin]
[Loaded com.example.ExamplePlugin$$Lambda$2/1111413685 from com.example.ExamplePlugin]
wilkinsona added a commit to spring-projects/spring-boot that referenced this issue Aug 14, 2018
Due to gradle/gradle#5510, using a lambda for a task action breaks
up-to-date checks in certain circumstances.

This commit updates JavaPluginAction to use an inner-class in place
of a lambda for the action that it adds to JavaCompile tasks. A test
has not been added as it does not appear to be possible to reproduce
it with a TestKit-based test.

Closes gh-14054
@david-noel

This comment has been minimized.

Copy link

@david-noel david-noel commented Aug 14, 2018

The --no-daemon option explains why I'm seeing different behavior between my local and Jenkins builds. My Jenkins build uses --no-daemon. When I use --no-daemon on my local build I get a different value from run to run. Using or not using --info seems to make no difference in my build. The numbers are always different.

@wolfs

This comment has been minimized.

Copy link
Member

@wolfs wolfs commented Aug 15, 2018

IIUC, then the lambda classes are created at execution time and are not created at compilation time by the compiler. The class name consists of the call site (com.example.ExamplePlugin$$Lambda$1), which seems to be stable, and a counter /837790893, which doesn't seem to be stable between different JVM runs.

The class is only created once per JVM, which explains why using the same daemon leaves the task up-to-date.

Would it make sense to drop the /837790893 suffix from the class name for up-to-date checks and build cache keys when the class comes from a lambda? Would that cause any changes to be missed, e.g. lead to wrong cache hits? It seems to be not the case...

@wilkinsona

This comment has been minimized.

Copy link
Contributor

@wilkinsona wilkinsona commented Aug 16, 2018

Unfortunately, the 1 in com.example.ExamplePlugin$$Lambda$1 means that it's the first lambda that's been created in the JVM, not that it's the first lambda in ExamplePlugin. If some code that uses a lambda runs first the number will change. For example:

[Loaded com.example.AnotherPlugin from file:/Users/awilkinson/.gradle/caches/jars-3/5b588771d431b79ceec34f122c58b6e8/buildSrc.jar]
[Loaded com.example.AnotherPlugin$$Lambda$1/322100932 from com.example.AnotherPlugin]
[Loaded com.example.AnotherPlugin$$Lambda$2/604480364 from com.example.AnotherPlugin]
[Loaded com.example.ExamplePlugin from file:/Users/awilkinson/.gradle/caches/jars-3/5b588771d431b79ceec34f122c58b6e8/buildSrc.jar]
[Loaded com.example.ExamplePlugin$$Lambda$3/1174599796 from com.example.ExamplePlugin]
[Loaded com.example.ExamplePlugin$$Lambda$4/893152673 from com.example.ExamplePlugin]

If you swap the order in which the plugins are applied, the numbers change:

[Loaded com.example.ExamplePlugin from file:/Users/awilkinson/.gradle/caches/jars-3/5b588771d431b79ceec34f122c58b6e8/buildSrc.jar]
[Loaded com.example.ExamplePlugin$$Lambda$1/836371508 from com.example.ExamplePlugin]
[Loaded com.example.ExamplePlugin$$Lambda$2/1073862849 from com.example.ExamplePlugin]
[Loaded com.example.AnotherPlugin from file:/Users/awilkinson/.gradle/caches/jars-3/5b588771d431b79ceec34f122c58b6e8/buildSrc.jar]
[Loaded com.example.AnotherPlugin$$Lambda$3/2091160281 from com.example.AnotherPlugin]
[Loaded com.example.AnotherPlugin$$Lambda$4/2127862399 from com.example.AnotherPlugin]

I can't see a way to derive a name from a lambda that will be stable across builds.

@wolfs

This comment has been minimized.

Copy link
Member

@wolfs wolfs commented Aug 16, 2018

@wilkinsona What Java version are you using? IIUC, there was a javac bug which was fixed and now the counter is per class: https://bugs.java.com/bugdatabase/view_bug.do?bug_id=8067422.

In your example it seems that the jar which contains the lambdas is not changing (buildSrc.jar). So maybe the Java bug above is something else. I saw the following code in InnerClassLambdaMetafactory:

lambdaClassName = targetClass.getName().replace('.', '/') + "$$Lambda$" + counter.incrementAndGet();

counter is a static field, so it would depend on the order the lambdas are loaded. That means that as there is change to the build file, the class name is stable (at least the part before the /).

@wilkinsona

This comment has been minimized.

Copy link
Contributor

@wilkinsona wilkinsona commented Aug 16, 2018

counter is a static field, so it would depend on the order the lambdas are loaded.

Yeah, that's the problem.

That means that as there is change to the build file, the class name is stable (at least the part before the /).

I don't think it is stable even with an unchanged build.gradle. For example, the order in which lambdas are loaded or the number of lambdas that are loaded could vary depending on some conditional logic and properties that have been passed into the build.

@ldaley

This comment has been minimized.

Copy link
Member

@ldaley ldaley commented Aug 16, 2018

configure-on-demand and composites are two other mechanics that can influence load order of configuration code.

Also, just adding a new plugin/script upstream of the lambda.

@wolfs

This comment has been minimized.

Copy link
Member

@wolfs wolfs commented Aug 21, 2018

If the target class of the lambda is serializable, we can obtain a SerializedLambda, which contains the actual method which will be invoked:

interface SerializableConsumer extends Consumer<String>, Serializable {}
SerializableConsumer serializableConsumer = (String x) -> System.out.println(x);
Method method = serializableConsumer.getClass().getDeclaredMethod("writeReplace");
method.setAccessible(true);
SerializedLambda serializedLambda = (SerializedLambda) method.invoke(serializableConsumer);

SerializedLambda has implClass and implMethodName as fields - those would be enough to track changes.

If the target class is not serializable, then a simpler form of the lambda is created.

@wolfs

This comment has been minimized.

Copy link
Member

@wolfs wolfs commented Aug 21, 2018

I guess we could parse the SAM method of the generated class for the lambda with asm and find out the actual invoked method.

@wolfs

This comment has been minimized.

Copy link
Member

@wolfs wolfs commented Aug 23, 2018

So it is possible to write a Java agent which changes LambdaMetaFactory to add an annotation to each generated lambda class with the necessary information. Here is a spike for the implementation: https://github.com/wolfs/lambda-meta-information-java-agent

Here is a test verifying that it works: https://github.com/wolfs/lambda-meta-information-java-agent/blob/master/instrumented-code/src/test/java/EnhancedLambdas.java#L17-L22

@wolfs

This comment has been minimized.

Copy link
Member

@wolfs wolfs commented Aug 23, 2018

On IBM JDK 8 the lambda generation is quite similar. The generated Lambda classes have names like <DeclaringClass>$$Lambda$154/00000000082FF1F0.

I think we could opt to drop everything after the last $ from the lambda class name. That should identify the lambda good enough and shield against most use cases. For anything else you would need to add actual inputs to the task, which I think would be good, too.

@wolfs wolfs changed the title [Build Cache] Adding doLast block to tasks with a lambda breaks cacheability A task with a doLast block using a lambda is never up-to-date Aug 23, 2018
@wolfs wolfs changed the title A task with a doLast block using a lambda is never up-to-date Task with a doLast block using a lambda is never up-to-date Aug 23, 2018
@lptr

This comment has been minimized.

Copy link
Member

@lptr lptr commented Aug 24, 2018

Is your suggestion to drop the last part of the lambda class name include using the agent or without it?

Just to be sure, you suggest that we retain only <DeclaringClass>$$Lambda from <DeclaringClass>$$Lambda$154/00000000082FF1F0? What if there are multiple lambdas declared by the same class?

bulldozer-bot bot added a commit to palantir/gradle-conjure that referenced this issue Mar 6, 2019
<!-- PR title should start with '[fix]', '[improvement]' or '[break]' if this PR would cause a patch, minor or major SemVer bump. Omit the prefix if this PR doesn't warrant a standalone release. -->

## Before this PR
<!-- Describe the problem you encountered with the current state of the world (or link to an issue) and why it's important to fix now. -->
Copying conjure sources was not cacheable because gradle does not (currently) support Java lambdas: gradle/gradle#5510

bluf of the issue: Java lambdas do not reproducibly generate the same ID internally, so gradle is unable to determine if the lambda has changed. The easy fix is to just use an anonymous function until gradle fixes this upstream.

## After this PR
<!-- Describe at a high-level why this approach is better. -->
Copying conjure sources is now cacheable
<!-- Reference any existing GitHub issues, e.g. 'fixes #000' or 'relevant to #000' -->
relevant to gradle/gradle#5510
JojOatXGME added a commit to JojOatXGME/jigsaw-gradle-plugin that referenced this issue Apr 4, 2019
Lambdas should not be used as task actions. They disable up-to-date
checks for the task. See
gradle/gradle#5510 (comment)

The options of task extensions must be registered as input manually. I
guess I should write a helper function for that to avoid inconsistency.
bulldozer-bot bot added a commit to palantir/gradle-baseline that referenced this issue Apr 12, 2019
## Before this PR

Devs could accidentally pass a lambda to `Task.doFirst` or `Task.doLast` actions in gradle plugins. This makes the task non-cacheable, as pointed out in gradle/gradle#5510.


## After this PR

==COMMIT_MSG=
Add an errorprone rule `GradleCacheableTaskAction` that prevents passing a lambda to `Task.doFirst` or `Task.doLast` when implementing gradle plugins.
==COMMIT_MSG==
@lptr lptr added @build-cache and removed in:build-cache labels Apr 16, 2019
@rgoldberg

This comment has been minimized.

Copy link
Contributor

@rgoldberg rgoldberg commented Sep 21, 2019

@mockitoguy Adding a doLast action in a build script will not disable caching/incremental build for the task. Only when the doLast action is implemented via a Java lambda. Groovy and Kotlin lambdas work just fine.

@wolfs: are you sure that Kotlin lambdas work just fine?

I have a plugin implemented completely in Kotlin that uses lambdas as arguments to doFirst() & doLast().

When including the plugin in a composite build, and applying that plugin to another included build, :common, running:

./gradlew --info --warning-mode all --console=plain build

Outputs the following to the console:

> Task :common:compileJava
Caching disabled for task ':common:compileJava' because:
  Build cache is disabled
Task ':common:compileJava' is not up-to-date because:
  Additional action for task ':common:compileJava': was implemented by the Java lambda 'org.gradle.java.taskconfigurer.JavaCompileTaskConfigurer$$Lambda$1406/0x0000000801ffb440'. Using Java lambdas is not supported, use an (anonymous) inner class instead.
All input files are considered out-of-date for incremental task ':common:compileJava'.
Full recompilation is required because no incremental change information is available. This is usually caused by clean builds or changing compiler arguments.
Compiling with JDK Java compiler API.
Created classpath snapshot for incremental compilation in 0.035 secs.
:common:compileJava (Thread[Execution worker for ':common',5,main]) completed. Took 1.773 secs.

org.gradle.java.taskconfigurer.JavaCompileTaskConfigurer is implemented in Kotlin, as are all the other classes in the plugin, so I don't know why Gradle thinks I'm using a Java lambda.

I thought that maybe the default value for the -jvm-target option for kotlinc (1.6) translates Kotlin lambdas to anonymous inner classes, but maybe 8 & higher translate to Java lambdas, but switching from 9 back to 1.6 still output the same info about Java lambdas.

Maybe older versions of Kotlin translate Kotlin lambdas into anonymous inner classes, but newer versions of Kotlin translate Kotlin lambdas into java lambdas.

I'm running using:

macOS 10.14.6 + all updates
Java 13: OpenJDK 64-Bit Server VM (build 13+33, mixed mode, sharing)
Gradle Wrapper 5.6.2 Kotlin DSL
Kotlin 1.3.50

With the following in my build.gradle.kts for the plugin:

tasks.withType<KotlinCompile>().configureEach {
    kotlinOptions {
        javaParameters = true
        jvmTarget      = VERSION_1_9.majorVersion
        verbose        = true
    }
}

As mentioned above, when I changed jvmTarget to "1.6", I still saw logs about Java lambdas, which doesn't make sense, as Java 1.6 doesn't support lambdas…

@wolfs

This comment has been minimized.

Copy link
Member

@wolfs wolfs commented Oct 7, 2019

Kotlin lambdas work fine, see #10854 (comment).

@rgoldberg

This comment has been minimized.

Copy link
Contributor

@rgoldberg rgoldberg commented Oct 7, 2019

Yeah. I was the one who opened that issue. I still haven’t gotten around to reporting the root issue that caused my project to use the Plugin Portal version instead of the version in my composite build. Sorry again for the confusion.

rschev added a commit to xenit-eu/dynamic-extensions-for-alfresco that referenced this issue Jan 17, 2020
Gradle has stopped doing up-to-date checks on tasks that use Java
lambdas. Using anonymous inner classes instead allows the up-to-date
checks to work again.

See gradle/gradle#5510 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.