XML formatter config is sometimes not applied when running gradle --parallel

[epkanol]

At this moment, don't know if this is a gradle or spotless-plugin problem.

Sometimes, when running gradle --parallel, the configFile() setting is not applied, or is applied too late (after formatting has started in another thread)

Command line used from Jenkins is: gradle --parallel build check --info

05:26:04 ------------------------------------------------------------
05:26:04 Gradle 5.6.3
05:26:04 ------------------------------------------------------------
05:26:04 
05:26:04 Build time:   2019-10-18 00:28:36 UTC
05:26:04 Revision:     bd168bbf5d152c479186a897f2cea494b7875d13
05:26:04 
05:26:04 Kotlin:       1.3.41
05:26:04 Groovy:       2.5.4
05:26:04 Ant:          Apache Ant(TM) version 1.9.14 compiled on March 12 2019
05:26:04 JVM:          1.8.0_231 (Oracle Corporation 25.231-b11)
05:26:04 OS:           Linux 3.10.0-1062.el7.x86_64 amd64
...
05:26:18 Downloading https://plugins.gradle.org/m2/com/diffplug/spotless/spotless-plugin-gradle/3.26.1/spotless-plugin-gradle-3.26.1.jar to XXXX
05:26:25 Downloading https://plugins.gradle.org/m2/com/diffplug/spotless/spotless-eclipse-base/3.2.1/spotless-eclipse-base-3.2.1.pom to XXXX
05:26:25 Downloading https://plugins.gradle.org/m2/com/diffplug/spotless/spotless-eclipse-wtp/3.15.1/spotless-eclipse-wtp-3.15.1.pom to XXXX

This is the spotless version:

classpath group: 'com.diffplug.spotless', name: 'spotless-plugin-gradle', version: '3.26.1'

This is the spotless configuration:

  spotless {
        java {
            googleJavaFormat('1.7')
            licenseHeaderFile file("$project.rootProject.projectDir/config/license.java")
        }
        format 'gradle', {
            target '**/*.gradle'
            trimTrailingWhitespace()
            indentWithSpaces(2)
            endWithNewline()
        }
        format 'xml', {
            target fileTree('.') {
                include '**/*.xml'
                exclude '**/build/**'
            }
            eclipseWtp('xml').configFile("$project.rootProject.projectDir/config/spotless.xml.prefs")
        }
    }

The xml prefs are the standard eclipse prefs:

user@host> cat config/spotless.xml.prefs
eclipse.preferences.version=1
indentationChar=space
indentationSize=2
lineWidth=200
user@host>

Error message (trimmed):

05:26:32 FAILURE: Build failed with an exception.
05:26:32 
05:26:32 * What went wrong:
05:26:32 Execution failed for task ':some-proj:spotlessXml'.
05:26:32 > The following files had format violations:
05:26:32       some-proj/path/some-file.xml
05:26:32           @@ -1,93 +1,117 @@
05:26:32            <?xml·version="1.0"·encoding="UTF-8"·standalone="no"?>
05:26:32           -<tag foo="value that is quite long and is likely to span several lines when formatted">
05:26:32           -··<!--·some comment·-->
05:26:32           -··<child-tag name="some-name"·value="123456789"·/>
05:26:32           +<tag
05:26:32           +\t\tname="value that is quite long and is likely to span several lines when formatted">
05:26:32           +\t\t<!--·some comment·-->
05:26:32           +\t\t<child-tag name="some-name"·value="123456789"·/>
...

It should be noted that the file was already formatted (as is seen in the above output, using two spaces) when this error was seen.
Something in either gradle or spotless causes the formatter to start formatting before it has received the correct service config (i.e. the contents of the property file).
I'd be happy to help to figure out what, if you could guide me into where to start looking (e.g. what are the entry points between the spotless plugin/ gradle configuration and the wtp formatter).

[nedtwigg]

I tried to trace out the execution below, under "Trace". Nothing jumped out at me as the problem, but I added a "My best guess" section, which is the subset of the full Trace which I think is most pertinent.

My best guess

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java

Lines 567 to 586 in b7f8c55

	public class EclipseWtpConfig {
	private final EclipseBasedStepBuilder builder;
	EclipseWtpConfig(EclipseWtpFormatterStep type, String version) {
	builder = type.createBuilder(GradleProvisioner.fromProject(getProject()));
	builder.setVersion(version);
	addStep(builder.build());
	}
	public void configFile(Object... configFiles) {
	requireElementsNonNull(configFiles);
	Project project = getProject();
	builder.setPreferences(project.files(configFiles).getFiles());
	replaceStep(builder.build());
	}
	}
	public EclipseWtpConfig eclipseWtp(EclipseWtpFormatterStep type) {
	return eclipseWtp(type, EclipseWtpFormatterStep.defaultVersion());
	}

spotless/lib-extra/src/main/java/com/diffplug/spotless/extra/EclipseBasedStepBuilder.java

Lines 118 to 121 in b7f8c55

	/** Set settings files containing Eclipse preferences */
	public void setPreferences(Iterable<File> settingsFiles) {
	this.settingsFiles = settingsFiles;
	}

Trace

Gradle configuration

The spotless { block delegates to SpotlessExtension.java. Each format (java, xml, etc) delegates to FormatExtension.java. For eclipseWtp, this is the code:

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java

Lines 567 to 586 in b7f8c55

	public class EclipseWtpConfig {
	private final EclipseBasedStepBuilder builder;
	EclipseWtpConfig(EclipseWtpFormatterStep type, String version) {
	builder = type.createBuilder(GradleProvisioner.fromProject(getProject()));
	builder.setVersion(version);
	addStep(builder.build());
	}
	public void configFile(Object... configFiles) {
	requireElementsNonNull(configFiles);
	Project project = getProject();
	builder.setPreferences(project.files(configFiles).getFiles());
	replaceStep(builder.build());
	}
	}
	public EclipseWtpConfig eclipseWtp(EclipseWtpFormatterStep type) {
	return eclipseWtp(type, EclipseWtpFormatterStep.defaultVersion());
	}

replaceStep is implemented like this:

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java

Lines 289 to 296 in b7f8c55

	/** Replaces the given step. */
	protected void replaceStep(FormatterStep replacementStep) {
	int existingIdx = getExistingStepIdx(replacementStep.getName());
	if (existingIdx == -1) {
	throw new GradleException("Cannot replace step '" + replacementStep.getName() + "' for spotless format '" + formatName() + "' because it hasn't been added yet.");
	}
	steps.set(existingIdx, replacementStep);
	}

In the afterEvaluate phase, this gets called:

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtension.java

Line 244 in b7f8c55

project.afterEvaluate(unused -> formatExtension.setupTask(spotlessTask));

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java

Lines 593 to 604 in b7f8c55

	protected void setupTask(SpotlessTask task) {
	task.setPaddedCell(paddedCell);
	task.setEncoding(getEncoding().name());
	task.setExceptionPolicy(exceptionPolicy);
	if (targetExclude == null) {
	task.setTarget(target);
	} else {
	task.setTarget(target.minus(targetExclude));
	}
	task.setSteps(steps);
	task.setLineEndingsPolicy(getLineEndings().createPolicy(getProject().getProjectDir(), () -> task.target));
	}

Gradle execution

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessTask.java

Lines 230 to 237 in b7f8c55

	// create the formatter
	try (Formatter formatter = Formatter.builder()
	.lineEndingsPolicy(lineEndingsPolicy)
	.encoding(Charset.forName(encoding))
	.rootDir(getProject().getRootDir().toPath())
	.steps(steps)
	.exceptionPolicy(exceptionPolicy)
	.build()) {

Step execution

spotless/lib-extra/src/main/java/com/diffplug/spotless/extra/wtp/EclipseWtpFormatterStep.java

Lines 51 to 53 in b7f8c55

	public EclipseBasedStepBuilder createBuilder(Provisioner provisioner) {
	return new EclipseBasedStepBuilder(NAME, " - " + toString(), provisioner, state -> formatterCall.apply(implementationClassName, state));
	}

spotless/lib-extra/src/main/java/com/diffplug/spotless/extra/EclipseBasedStepBuilder.java

Lines 118 to 121 in b7f8c55

	/** Set settings files containing Eclipse preferences */
	public void setPreferences(Iterable<File> settingsFiles) {
	this.settingsFiles = settingsFiles;
	}

[epkanol]

Tried to reproduce the issue, both within the IDE and on our CI env, but failed.
Did manage to get somewhat deeper into the code, though, so adding my thoughts here, in case it helps someone:

The EclipseWtpConfig object first registers itself within the enclosing FormatExtension in the constructor (using the default parameters):

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java

Line 573 in b7f8c55

addStep(builder.build());

Then, in the configFile() method, it replaces itself with a properly configured version:

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java

Line 580 in b7f8c55

replaceStep(builder.build());

The state to the formatter is calculated lazily, though the builder is mutable (has the call to setPreferences on the line above:

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java

Line 579 in b7f8c55

builder.setPreferences(project.files(configFiles).getFiles());

But presumably this is not a problem, a friend who knows a lot about Gradle says that the configuration phase is single-threaded. Still, the error matches the description of one "default-configured" object overwriting a properly configured one.

The steps in the FormatExtension are copied to the SpotlessTask a bit further down:

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java

Line 602 in b7f8c55

task.setSteps(steps);

Tell you the truth, I have no idea why our execution failed to pick up the configFile setting in one of our CI runs. And right now, I cannot reproduce it.
Should also mention, we do not use "configuration on demand" for the CI tasks (as we are using --parallel, and we have had issues with interference in other plugins, mostly own-developed)
For now, we'll resort to running these jobs without the --parallel flag.
Will call back if the issue resurfaces.

[nedtwigg]

Sounds good. I was surprised at this bug, because (as you say) gradle configuration is single-threaded even for parallel builds, and I've used parallel myself for a long time without problems. Absolutely possible there's a bug somewhere (global search for static fields might bring up an ill-advised cache somewhere), so we'll leave this open in case you or someone else gets any more whiffs of a parallel race condition bug.

[fvgh]

Sorry, thought that it was related to an issue I found. But it is not.

[fvgh]

OK, while preparing quite some cleanup, I finally found a remaining bug in the XML formatter, which had not been addressed by #489 .
I am afraid when I did the initial eclipse-base configuration interface I assumed that there is not much to do for the individual formatters. This assumption was entirely wrong and let to an unreadable and error-prone interface implementation on WTP side. Hence it was always on my TODO list for refactoring.
To summarize: I am afraid all WTP formatters don't work reliable when instances of the same type used in parallel. This does not affect usage of multiple types, since CSS, XML, ... formatters live in individual class loaders.

Together with the new eclipse-base configuration interface, I'll provide a final solution for these shortcomings.

@epkanol Sorry about the trouble caused.

[fvgh]

I had another look at the Eclipse source. Currently the Spotless WTP provides the possibility to configure multiple formatter settings for the same formatter. For example you can configure in Gradle an XML formatter for XML files in directory foo with different formatter settings than for XML files in directory bar. I am afraid I cannot make this thread safe within the WTP. This possibility is already forbidden for CSS, since the formatter loads the settings differently. For the CSS formatter the user gets already an exception when he/she tries to use different properties.
I see only the following options:

1. Disallow different configurations for all formatters, like it is done already for CSS.
2. Use different class loaders for each instance. Currently we have only different class-loaders per type (CSS, XML, HTML, ...)

I actually think it is cleaner to disallow different configurations for all WTP formatters. I am not sure whether this option is used in real life.

Any thoughts?

[epkanol]

Thanks for having a look at this!

For our purposes, we'd be happy with having a single formatting configuration for the whole project.
In certain circumstances (e.g. large test data files) we simply ignore formatting altogether.
So I wouldn't recommend going the classloader route (this usually has all sorts of side effects, e.g. enums that "occur in multiple instances") and similar issues.
Throwing an exception when different properties are detected seems reasonable for me.
The only issue I could think of is to determine wheter to simply throw an exception when a second property files is encountered, or whether to actually check the properties, and only throw if they are the same.
But I'd argue that suppressing the exception just because properties are the same leads to an confusing user experience (as people most likely are not aware that different properties need to be the same for spotless to work). So I'd recommend simply throwing the exception whenever a ready-configured XML formatter encounters new properties (i.e. configuration must be applied once, and only once).

I probably should mention that currently we only use XML formatting.
So in our view, it's reasonable for spotless/WTP to support ONE formatting configuration for XML, and another ONE for CSS, and so on...

[nedtwigg]

I'm fine with anything, IMO the easiest path is:

• stick with our current one-classloader approach, even though it constrains multiple configs
• if that ends up being a problem, then we can tackle adding support for multiple classloaders