Rewrote the PropertiesProvider API to support initialization, close and better multithreading #339

mathieucarbou · 2022-03-28T12:25:10Z

Correctly initialize and close the resources used in properties providers.
Better support multi-threading
Remove any contention created by synchronized

Ref: #335 | #327

CC: @philippn

mathieucarbou · 2022-03-28T13:23:49Z

Note: still need to be improved because it will slow down...

mathieucarbou · 2022-03-28T15:14:14Z

@hazendaz @dbwiddis : ready for review!

mathieucarbou · 2022-03-28T16:13:12Z

license-maven-plugin/src/main/java/com/mycila/maven/plugin/license/PropertiesProvider.java

+   * @deprecated Use instead {@link #adjustProperties(AbstractLicenseMojo, Map, Document)}
+   */
+  @Deprecated
+  default Map<String, String> getAdditionalProperties(AbstractLicenseMojo mojo, Properties currentProperties, Document document) {


kept for backward compat' just in case...

mathieucarbou · 2022-03-28T16:16:37Z

license-maven-plugin/src/main/java/com/mycila/maven/plugin/license/PropertiesProvider.java

+  }
+
+  @Override
+  default void close() {}


mathieucarbou · 2022-03-28T16:16:41Z

license-maven-plugin/src/main/java/com/mycila/maven/plugin/license/PropertiesProvider.java

-  Map<String, String> getAdditionalProperties(AbstractLicenseMojo mojo, Properties currentProperties, Document document);
+public interface PropertiesProvider extends Closeable {
+
+  default void init(AbstractLicenseMojo mojo, Map<String, String> currentProperties) {}


...en-plugin-git/src/main/java/com/mycila/maven/plugin/license/git/CopyrightAuthorProvider.java

...ven-plugin-git/src/main/java/com/mycila/maven/plugin/license/git/CopyrightRangeProvider.java

license-maven-plugin-git/src/main/java/com/mycila/maven/plugin/license/git/GitLookup.java

mathieucarbou · 2022-03-28T16:24:17Z

...aven-plugin-svn/src/main/java/com/mycila/maven/plugin/license/svn/SVNPropertiesProvider.java

+      // One-time warning for shallow repo
+      if (mojo.warnIfShallow && !warnedIfShallow.get()) {
+        SVNInfo info = svnClientManager.getWCClient().doInfo(documentFile, SVNRevision.HEAD);
+        if (info.getDepth() != SVNDepth.INFINITY) {
+          if (warnedIfShallow.compareAndSet(false, true)) {
+            mojo.warn(
+                "Sparse svn repository detected. Year and author property values may not be accurate.");


@dbwiddis : FYI warnedIfShallow code updated

When I first put this in I didn't realize the SVN provider didn't have the same set of properties. There are no authors to consider, and only the "last change" year is relevant (so a sparse repo may miss the latest commit of a file and default to current year). So this can probably just say "year" values like the adjusted git one above.

mathieucarbou · 2022-03-28T16:26:12Z

...aven-plugin-svn/src/main/java/com/mycila/maven/plugin/license/svn/SVNPropertiesProvider.java

+  private final AtomicBoolean warnedIfShallow = new AtomicBoolean();
+  private final Queue<SVNClientManager> clients = new ConcurrentLinkedQueue<>();
+  private final ThreadLocal<SimpleDateFormat> sdfTimestampThreadLocal = ThreadLocal.withInitial(
+      () -> new SimpleDateFormat("yyyyMMdd-HH:mm:ss"));
+  private final ThreadLocal<SVNClientManager> svnClientThreadLocal = ThreadLocal.withInitial(() -> {
+    SVNClientManager svnClientManager = svnCredentials == null ?
+        SVNClientManager.newInstance(new DefaultSVNOptions()) :
+        SVNClientManager.newInstance(new DefaultSVNOptions(), svnCredentials.getLogin(),
+            svnCredentials.getPassword());
+    clients.offer(svnClientManager);
+    return svnClientManager;
+  });


Previously incorrect resource cleanup: thread-local SVN clients were not closed. Kee ing track of all created clients per thread to clean them at the end.

mathieucarbou · 2022-03-28T16:27:09Z

license-maven-plugin/src/main/java/com/mycila/maven/plugin/license/AbstractLicenseMojo.java

+    Map<String, String> globalProperties = getDefaultProperties();

-        for (final Map.Entry<String, String> entry : mergeProperties(licenseSet, document).entrySet()) {
-          if (entry.getValue() != null) {
-            props.setProperty(entry.getKey(), entry.getValue());
-          } else {
-            props.remove(entry.getKey());
-          }
-        }
-        for (final PropertiesProvider provider : propertiesProviders) {
-          try {
-            final Map<String, String> providerProperties = provider.getAdditionalProperties(AbstractLicenseMojo.this, props, document);
-            if (getLog().isDebugEnabled()) {
-              getLog().debug("provider: " + provider.getClass() + " brought new properties\n" + providerProperties);
-            }
-            for (Map.Entry<String, String> entry : providerProperties.entrySet()) {
-              if (entry.getValue() != null) {
-                props.setProperty(entry.getKey(), entry.getValue());
-              } else {
-                props.remove(entry.getKey());
-              }
-            }
-          } catch (Exception e) {
-            getLog().warn("failure occurred while calling " + provider.getClass(), e);
-          }
+    // we override by properties in the licenseSet
+    if (licenseSet.properties != null) {
+      for (Map.Entry<String, String> entry : licenseSet.properties.entrySet()) {
+        if (!System.getProperties().contains(entry.getKey())) {
+          globalProperties.put(entry.getKey(), entry.getValue());


Avoid executing this code for each file.

mathieucarbou · 2022-03-28T16:27:43Z

license-maven-plugin/src/main/java/com/mycila/maven/plugin/license/AbstractLicenseMojo.java

+
+      for (final PropertiesProvider provider : ServiceLoader.load(PropertiesProvider.class,
+          Thread.currentThread().getContextClassLoader())) {
+        provider.init(this, globalProperties);


init called here

mathieucarbou · 2022-03-28T16:27:59Z

license-maven-plugin/src/main/java/com/mycila/maven/plugin/license/AbstractLicenseMojo.java

@@ -685,6 +720,7 @@ public Properties load(final Document document) {

    } finally {
      executorService.shutdownNow();
+      propertiesProviders.forEach(PropertiesProvider::close);


close called here

dbwiddis

LGTM. One suggested text change.

dbwiddis · 2022-03-28T23:43:56Z

...aven-plugin-svn/src/main/java/com/mycila/maven/plugin/license/svn/SVNPropertiesProvider.java

+      // One-time warning for shallow repo
+      if (mojo.warnIfShallow && !warnedIfShallow.get()) {
+        SVNInfo info = svnClientManager.getWCClient().doInfo(documentFile, SVNRevision.HEAD);
+        if (info.getDepth() != SVNDepth.INFINITY) {
+          if (warnedIfShallow.compareAndSet(false, true)) {
+            mojo.warn(
+                "Sparse svn repository detected. Year and author property values may not be accurate.");


When I first put this in I didn't realize the SVN provider didn't have the same set of properties. There are no authors to consider, and only the "last change" year is relevant (so a sparse repo may miss the latest commit of a file and default to current year). So this can probably just say "year" values like the adjusted git one above.

…nd multithreading better

mathieucarbou added is:bug Bugs to fix in:git MLP Git integration labels Mar 28, 2022

mathieucarbou added this to the 4.2 milestone Mar 28, 2022

mathieucarbou requested review from hazendaz and dbwiddis March 28, 2022 12:25

mathieucarbou self-assigned this Mar 28, 2022

mathieucarbou force-pushed the closing-providers branch 2 times, most recently from 1d7e972 to ef7866d Compare March 28, 2022 12:52

mathieucarbou marked this pull request as draft March 28, 2022 13:23

mathieucarbou force-pushed the closing-providers branch from ef7866d to 4860641 Compare March 28, 2022 15:13

mathieucarbou changed the title ~~Correctly close the repository~~ Rewrote the PropertiesProvider API to support initialization, close and multithreading better Mar 28, 2022

mathieucarbou marked this pull request as ready for review March 28, 2022 15:14

This was referenced Mar 28, 2022

Fix unclosed reader (reported by SonarLint) #335

Closed

Enforce coding style #330

Closed

mathieucarbou force-pushed the closing-providers branch from 4860641 to 03fe1a7 Compare March 28, 2022 16:10