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

Custom ValueSource erroneously tracks files etc used in obtain() when ValueSource parameters are passed in #22305

Closed
cloudshiftchris opened this issue Oct 4, 2022 · 4 comments
Assignees
Milestone

Comments

@cloudshiftchris
Copy link

cloudshiftchris commented Oct 4, 2022

When creating a custom ValueSource with parameters, and using other ValueSource instances for those parameters, the inputs are erroneously tracked.

Expected Behavior

As documented, ValueSource implementations should not track files, environment variables, etc.

Current Behavior

Gradle erroneously tracks ValueSource inputs, resulting in unexpectedly dirty configuration caches.

Context

This causes the configuration cache to miss 100% of the time.

This is due to a collision of events:
- Gradle fingerprints the input parameters to the ValueSource;
- this results in beforeValueObtained() event being fired, which DISABLES input tracking
- ...and then afterValueObtained() is fired, which ENABLES input tracking, just in time to drop into the obtain()
method of our custom ValueSource

Steps to Reproduce

Reproduced here. Consistently reproducible in configurations where a ValueSource is used as a parameter to a ValueSource

Your Environment

MacOS, Gradle 7.5.1, Java 17

@cloudshiftchris
Copy link
Author

At a minimum it should be documented that ValueSources should not be used as parameters to ValueSources; this is likely insufficient as it's not directly evident what constitutes a 'ValueSource' - it's intuitive to pass in providers.gradleProperty, for example.

A hard error when this is detected would be a good initial step.

Ideally the inputTracking state would be smart enough to support these use cases.

@cloudshiftchris
Copy link
Author

cloudshiftchris commented Oct 4, 2022

Added additional diagnostic output to show the input tracking state:

        beforeValueObtained()  // input tracking turned off (entering custom MyValueSource)
        beforeValueObtained()  // input tracking turned off (entering environment variable)
        afterValueObtained()   // input tracking turned on  (exiting environment variable)
        valueObtained(): class org.gradle.api.internal.provider.sources.EnvironmentVariableValueSource org.gradle.api.internal.provider.sources.EnvironmentVariableValueSource$Inject@46d4f344

        // whoops; inside our ValueSource with input tracking turned on
        In value source
        Test file @ /Users/chrislee/.cloudshift/test.properties

        afterValueObtained()   // input tracking turned on (exiting custom MyValueSource)
        valueObtained(): class Build_gradle$MyValueSource Build_gradle$MyValueSource$Inject@6383cdd1

cloudshiftchris added a commit to cloudshiftchris/gradle that referenced this issue Oct 4, 2022
@cloudshiftchris
Copy link
Author

This looks to be fixable in org.gradle.configurationcache.fingerprint.ConfigurationCacheFingerprintWriter by changing the ThreadLocal to a counter (e.g. ThreadLocal) to support nested ValueSources, with a state of '0' -> input tracking enabled and < 0 (assuming beforeValueObtained() decrements counter) -> input tracking disabled.

@mlopatkin mlopatkin self-assigned this Oct 24, 2022
bot-gradle added a commit that referenced this issue Oct 25, 2022
This PR addresses some discrepancies in how the configuration cache input tracking works inside the ValueSource and `buildscan.background` callback. Prior:
1. `ValueSource.obtain` was complaining about the use of the standard Java process API
2. `buildscan.background` was complaining about the use of `ExecOperations`.

The discrepancy was coming from the fact that there were two ways of disabling input tracking - removing the instrumentation "interceptor" temporarily (used by `background`), and ignoring the calls in the input listener in the `ConfigurationCacheFingerprintWriter` (used by ValueSource). This PR removes the first way, extracts the state of the input tracking into a separate `InputTrackingState` class, and switches `background` to use this new class. Additionally, as the external process warning is closely tied to the input tracking, the `ConfigurationCacheProblemsListener` also takes input tracking state into account when deciding whether to emit a problem if an external process is started.

The remaining corner cases are related to ValueSources evaluated at the configuration time. Evaluating a value source inside the `background` block may still make this source a configuration input. The workaround is to use Java APIs directly to obtain the value instead.

Fixes #22305

Co-authored-by: Mikhail Lopatkin <mlopatkin@gradle.com>
@mlopatkin mlopatkin added this to the 7.6 RC1 milestone Oct 25, 2022
@mlopatkin
Copy link
Member

This will be fixed in 7.6 RC1. There is some follow-up work to do, tracked in #22494. Big thanks to @cloudshiftchris for finding the root cause!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

2 participants