Build system information should be optionally set

[ryantse]

Changes the build system information to be conditionally set within the properties. In multi-threaded builds, without the optional put, a ConcurrentModificationException occurs as this information is written at the same time the object is being iterated over.

Context

Fixes #469.

Contributor Checklist

• Added relevant integration or unit tests to verify the changes
• Update the Readme or any other documentation (including relevant Javadoc)
• Ensured that tests pass locally: mvn clean package
• Ensured that the code meets the current checkstyle coding style definition: mvn clean verify -Pcheckstyle -Dmaven.test.skip=true -B

[joschi]

@ryantse As far as I see maybePut is also not thread-safe, so even with the change, we would see ConcurrentModificationException in some cases.

If it was possible at all, it would be great if we could get the build server data only once and then share it with the invocations of the plugin in the Maven sub-modules.

Otherwise making BuildServerDataProvider thread-safe would be an option too.

[ryantse]

@joschi: Thanks for the quick feedback. You're right that the original patch didn't fully correct the thread-safety issue. The only time this really is a problem is when injectAllReactorProjects is turned on along with additional properties being configured in sub-modules.

I have pushed another patch that more comprehensively addresses the issue. This patch removes all usage of the underlying HashMap functions in preference for equivalents in the Properties class. As Properties is properly synchronized, this mitigates all instances where multiple modules (and thus multiple threads) share the same object.

[ryantse]

@TheSnoozer: Could you take a look at this pull request as well?

[ryantse]

@TheSnoozer: Can this be merged or do you have any feedback on this patch? Thanks in advance!

[TheSnoozer]

Hi,
sorry for the delay here. Thanks for creating the Merge-Request with the needed changes. From what I can tell they look reasonable for me. Would this perhaps already mean we need to cut a 4.0.2 release (e.g. #507)? Perhaps could you confirm the changes fixed the issue locally? If so I'd just cut a release on the weekend.

[ryantse]

@TheSnoozer: I was able to test the snapshot build of the plugin with our build system and it works well in my environment. It would be great if you could release a 4.0.2 with these changes.

[TheSnoozer]

Thanks for confirming! Will do a release on the weekend.

[TheSnoozer]

Now available as https://repo1.maven.org/maven2/pl/project13/maven/git-commit-id-plugin/4.0.2/. Thanks again for the investigation and pushing for the solution!