Uploaded image for project: 'Maven'
  1. Maven
  2. MNG-6105

properties.internal.SystemProperties.addSystemProperties() is not really thread-safe

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.3.9
    • Fix Version/s: 3.5.0-alpha-1, 3.5.0
    • Component/s: core
    • Labels:
      None
    • Environment:

      Description

      We got a rather large Maven project with a couble of modules that execute maven-assembly-plugin. Our build runs in parallel mode via -Tx.
      From time to time we are seeing following exception causing the respective build to fail:

      [ERROR] Failed to execute goal org.apache.maven.plugins:maven-assembly-plugin:2.6:single (assembly-zip) on project some-module: Execution assembly-zip of goal org.apache.maven.plugins:maven-assembly-plugin:2.6:single failed. NullPointerException -> [Help 1]
      org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-assembly-plugin:2.6:single (assembly-zip) on project some-module: Execution assembly-zip of goal org.apache.maven.plugins:maven-assembly-plugin:2.6:single failed.
      	at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:212)
      	at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:153)
      	at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:145)
      	at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject(LifecycleModuleBuilder.java:116)
      	at org.apache.maven.lifecycle.internal.builder.multithreaded.MultiThreadedBuilder$1.call(MultiThreadedBuilder.java:185)
      	at org.apache.maven.lifecycle.internal.builder.multithreaded.MultiThreadedBuilder$1.call(MultiThreadedBuilder.java:181)
      	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
      	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
      	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
      	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
      	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
      	at java.lang.Thread.run(Thread.java:745)
      Caused by: org.apache.maven.plugin.PluginExecutionException: Execution assembly-zip of goal org.apache.maven.plugins:maven-assembly-plugin:2.6:single failed.
      	at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:145)
      	at org.apache.maven.lifecycle.internal.MojoExecutor.execute(MojoExecutor.java:207)
      	... 11 more
      Caused by: java.lang.NullPointerException
      	at java.util.Hashtable.put(Hashtable.java:459)
      	at org.apache.maven.properties.internal.SystemProperties.addSystemProperties(SystemProperties.java:38)
      	at org.apache.maven.project.artifact.MavenMetadataSource.getSystemProperties(MavenMetadataSource.java:756)
      	at org.apache.maven.project.artifact.MavenMetadataSource.retrieveRelocatedProject(MavenMetadataSource.java:574)
      	at org.apache.maven.project.artifact.MavenMetadataSource.retrieve(MavenMetadataSource.java:190)
      	at org.apache.maven.repository.legacy.resolver.DefaultLegacyArtifactCollector.recurse(DefaultLegacyArtifactCollector.java:532)
      	at org.apache.maven.repository.legacy.resolver.DefaultLegacyArtifactCollector.recurse(DefaultLegacyArtifactCollector.java:584)
      	at org.apache.maven.repository.legacy.resolver.DefaultLegacyArtifactCollector.collect(DefaultLegacyArtifactCollector.java:144)
      	at org.apache.maven.artifact.resolver.DefaultArtifactResolver.resolve(DefaultArtifactResolver.java:493)
      	at org.apache.maven.artifact.resolver.DefaultArtifactResolver.resolveWithExceptions(DefaultArtifactResolver.java:348)
      	at org.apache.maven.artifact.resolver.DefaultArtifactResolver.resolveTransitively(DefaultArtifactResolver.java:342)
      	at org.apache.maven.artifact.resolver.DefaultArtifactResolver.resolveTransitively(DefaultArtifactResolver.java:321)
      	at org.apache.maven.artifact.resolver.DefaultArtifactResolver.resolveTransitively(DefaultArtifactResolver.java:286)
      	at org.apache.maven.plugin.assembly.artifact.DefaultDependencyResolver.resolveTransitively(DefaultDependencyResolver.java:253)
      	at org.apache.maven.plugin.assembly.artifact.DefaultDependencyResolver.resolveDependencySets(DefaultDependencyResolver.java:169)
      	at org.apache.maven.plugin.assembly.archive.phase.DependencySetAssemblyPhase.execute(DependencySetAssemblyPhase.java:94)
      	at org.apache.maven.plugin.assembly.archive.DefaultAssemblyArchiver.createArchive(DefaultAssemblyArchiver.java:178)
      	at org.apache.maven.plugin.assembly.mojos.AbstractAssemblyMojo.execute(AbstractAssemblyMojo.java:484)
      	at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:134)
      	... 12 more
      [ERROR] 
      [ERROR] Re-run Maven using the -X switch to enable full debug logging.
      [ERROR] 
      [ERROR] For more information about the errors and possible solutions, please read the following articles:
      [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/PluginExecutionException
      

      There seems to be concurrent system property removal while properties.internal.SystemProperties is iterating over the previously calculated property keys:

          /**
           * Thread-safe System.properties copy implementation.
           *
           * @see "https://issues.apache.org/jira/browse/MNG-5670"
           */
          public static void addSystemProperties( Properties props )
          {
              for ( String key : System.getProperties().stringPropertyNames() )
              {
                  props.put( key, System.getProperty( key ) );
              }
      }
      

      System.getProperty( key ) possibly yields a null value which Hashtable/Properties does not accept.

      Possible solutions:

      • refrain from adding null by explicitly checking for it
      • replace for-loop with props.addAll(System.getProperties().clone())

        Issue Links

          Activity

          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build maven-3.x #1519 (See https://builds.apache.org/job/maven-3.x/1519/)
          MNG-6105 properties.internal.SystemProperties.addSystemProperties() is (gboue: http://git-wip-us.apache.org/repos/asf/?p=maven.git&a=commit&h=5b4b8bd94c87afd2a1527d6a860e9673bdaf4a22)

          • (edit) maven-aether-provider/src/main/java/org/apache/maven/repository/internal/MavenRepositorySystemUtils.java
          • (edit) maven-core/src/main/java/org/apache/maven/properties/internal/SystemProperties.java
          • (edit) maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuildingRequest.java
          • (edit) maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuildingRequest.java
          • (edit) maven-core/src/main/java/org/apache/maven/execution/DefaultMavenExecutionRequest.java
          • (edit) maven-settings-builder/src/main/java/org/apache/maven/settings/building/DefaultSettingsBuildingRequest.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build maven-3.x #1519 (See https://builds.apache.org/job/maven-3.x/1519/ ) MNG-6105 properties.internal.SystemProperties.addSystemProperties() is (gboue: http://git-wip-us.apache.org/repos/asf/?p=maven.git&a=commit&h=5b4b8bd94c87afd2a1527d6a860e9673bdaf4a22 ) (edit) maven-aether-provider/src/main/java/org/apache/maven/repository/internal/MavenRepositorySystemUtils.java (edit) maven-core/src/main/java/org/apache/maven/properties/internal/SystemProperties.java (edit) maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuildingRequest.java (edit) maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuildingRequest.java (edit) maven-core/src/main/java/org/apache/maven/execution/DefaultMavenExecutionRequest.java (edit) maven-settings-builder/src/main/java/org/apache/maven/settings/building/DefaultSettingsBuildingRequest.java
          Hide
          stephenc Stephen Connolly added a comment -

          Maven 3.4.0 has been dropped. See this thread for more details.

          This issue will need to be re-scheduled for a Maven release in the (hopefully near) future.

          Show
          stephenc Stephen Connolly added a comment - Maven 3.4.0 has been dropped. See this thread for more details. This issue will need to be re-scheduled for a Maven release in the (hopefully near) future.
          Hide
          famod Falko Modler added a comment -

          Sorry for not replying. I did not have the time to test the fix (and it is very hard to reproduce).
          I will create a new ticket in case to problem pops up again with Maven 3.4.0+.

          Show
          famod Falko Modler added a comment - Sorry for not replying. I did not have the time to test the fix (and it is very hard to reproduce). I will create a new ticket in case to problem pops up again with Maven 3.4.0+.
          Hide
          gboue Guillaume Boué added a comment -

          All right, the issue is fixed to me yes.

          Show
          gboue Guillaume Boué added a comment - All right, the issue is fixed to me yes.
          Hide
          michael-o Michael Osipov added a comment -

          Guillaume, if you consider it fixed and the OP hasn't reacted for two months, consider it fixed and target for 3.4.0.

          Show
          michael-o Michael Osipov added a comment - Guillaume, if you consider it fixed and the OP hasn't reacted for two months, consider it fixed and target for 3.4.0.
          Hide
          gboue Guillaume Boué added a comment -

          Can you test with the latest snapshot of maven-core and see if you still hit this issue? Thanks.

          Show
          gboue Guillaume Boué added a comment - Can you test with the latest snapshot of maven-core and see if you still hit this issue? Thanks.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build maven-3.x #1398 (See https://builds.apache.org/job/maven-3.x/1398/)
          MNG-6105 properties.internal.SystemProperties.addSystemProperties() is (gboue: rev ace448158131285e5ef8fb54b96dfb3d8d05f37e)

          • (edit) maven-aether-provider/src/main/java/org/apache/maven/repository/internal/MavenRepositorySystemUtils.java
          • (edit) maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuildingRequest.java
          • (edit) maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuildingRequest.java
          • (edit) maven-settings-builder/src/main/java/org/apache/maven/settings/building/DefaultSettingsBuildingRequest.java
          • (edit) maven-core/src/main/java/org/apache/maven/execution/DefaultMavenExecutionRequest.java
          • (edit) maven-core/src/main/java/org/apache/maven/properties/internal/SystemProperties.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build maven-3.x #1398 (See https://builds.apache.org/job/maven-3.x/1398/ ) MNG-6105 properties.internal.SystemProperties.addSystemProperties() is (gboue: rev ace448158131285e5ef8fb54b96dfb3d8d05f37e) (edit) maven-aether-provider/src/main/java/org/apache/maven/repository/internal/MavenRepositorySystemUtils.java (edit) maven-model-builder/src/main/java/org/apache/maven/model/building/DefaultModelBuildingRequest.java (edit) maven-core/src/main/java/org/apache/maven/project/DefaultProjectBuildingRequest.java (edit) maven-settings-builder/src/main/java/org/apache/maven/settings/building/DefaultSettingsBuildingRequest.java (edit) maven-core/src/main/java/org/apache/maven/execution/DefaultMavenExecutionRequest.java (edit) maven-core/src/main/java/org/apache/maven/properties/internal/SystemProperties.java
          Hide
          schulte77 Christian Schulte added a comment -

          Should work as well. You need to change the API that way.

          Show
          schulte77 Christian Schulte added a comment - Should work as well. You need to change the API that way.
          Hide
          gboue Guillaume Boué added a comment - - edited

          All methods of Properties itself (and Hashtable from which it inherits) are internally synchronized on this so I think the guarantee is made.

          Show
          gboue Guillaume Boué added a comment - - edited All methods of Properties itself (and Hashtable from which it inherits) are internally synchronized on this so I think the guarantee is made.
          Hide
          michael-o Michael Osipov added a comment -

          This would only work, imho, if someone else synchronizes on properties when addProperty is called. We cannot guarantee that. Am I wrong?

          Show
          michael-o Michael Osipov added a comment - This would only work, imho, if someone else synchronizes on properties when addProperty is called. We cannot guarantee that. Am I wrong?
          Hide
          gboue Guillaume Boué added a comment -

          So the issue is about copying one Properties to another. Why would synchronizing on the source property instance not be enough? I.e, following code:

          public static Properties copyProperties( Properties properties )
          {
              final Properties copyProperties = new Properties();
              synchronized ( properties )
              {
                  copyProperties.putAll( properties );
              }
              return copyProperties;
          }
          

          I ran all core-its with this (both on Windows 10 and Ubuntu 16.04) and didn't have any issues. I also ran a build of maven-plugins-aggregator with -T 5. Since it synchronizes on the given properties, it makes sure no other thread can remove a key or set a different value to a key during the iteration in putAll. I also noticed this is what some Oracle internal code does (relevant mail).

          Show
          gboue Guillaume Boué added a comment - So the issue is about copying one Properties to another. Why would synchronizing on the source property instance not be enough? I.e, following code: public static Properties copyProperties( Properties properties ) { final Properties copyProperties = new Properties(); synchronized ( properties ) { copyProperties.putAll( properties ); } return copyProperties; } I ran all core-its with this (both on Windows 10 and Ubuntu 16.04) and didn't have any issues. I also ran a build of maven-plugins-aggregator with -T 5 . Since it synchronizes on the given properties, it makes sure no other thread can remove a key or set a different value to a key during the iteration in putAll . I also noticed this is what some Oracle internal code does ( relevant mail ).
          Hide
          schulte77 Christian Schulte added a comment - - edited

          Something to provide via 'maven-shared-utils' then. BTW: You don't need to mind any performance issues here. Nothing about this is a hotspot or anything like that. The synchronization during load is performed on a method local object no other thread can ever access during loading.

          Show
          schulte77 Christian Schulte added a comment - - edited Something to provide via 'maven-shared-utils' then. BTW: You don't need to mind any performance issues here. Nothing about this is a hotspot or anything like that. The synchronization during load is performed on a method local object no other thread can ever access during loading.
          Hide
          gboue Guillaume Boué added a comment -

          Interesting indeed, hadn't thought of that.

          I think there is small difference that makes this solution better: if another thread removes a system property during the internal iteration in store, it would still be written to the stream due to the synchronization and loaded after to the copy, so it effectively contains what the initial system properties were. With a null-check, if another thread removed a key after stringPropertyNames was called, it won't be added in the copy since the value is retrieved during iteration. Just wondering, is there any other reason this is preferable over the null-check?

          On another hand, it adds synchronization at the load call, when only store really needs to be synchronized, but I don't think this is really an issue. Will sleep on that and make code changes tomorrow.

          maven-settings-builder is also affected. There should be a way to factor out this code but I didn't spot a good place for it.

          Show
          gboue Guillaume Boué added a comment - Interesting indeed, hadn't thought of that. I think there is small difference that makes this solution better: if another thread removes a system property during the internal iteration in store , it would still be written to the stream due to the synchronization and loaded after to the copy, so it effectively contains what the initial system properties were. With a null-check, if another thread removed a key after stringPropertyNames was called, it won't be added in the copy since the value is retrieved during iteration. Just wondering, is there any other reason this is preferable over the null-check? On another hand, it adds synchronization at the load call, when only store really needs to be synchronized, but I don't think this is really an issue. Will sleep on that and make code changes tomorrow. maven-settings-builder is also affected. There should be a way to factor out this code but I didn't spot a good place for it.
          Hide
          schulte77 Christian Schulte added a comment -

          I'd do it that way:

          package org.apache.maven.properties.internal;
          
          /*
           * Licensed to the Apache Software Foundation (ASF) under one
           * or more contributor license agreements.  See the NOTICE file
           * distributed with this work for additional information
           * regarding copyright ownership.  The ASF licenses this file
           * to you under the Apache License, Version 2.0 (the
           * "License"); you may not use this file except in compliance
           * with the License.  You may obtain a copy of the License at
           *
           *   http://www.apache.org/licenses/LICENSE-2.0
           *
           * Unless required by applicable law or agreed to in writing,
           * software distributed under the License is distributed on an
           * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
           * KIND, either express or implied.  See the License for the
           * specific language governing permissions and limitations
           * under the License.
           */
          
          import java.io.ByteArrayInputStream;
          import java.io.ByteArrayOutputStream;
          import java.io.IOException;
          import java.util.Properties;
          
          /**
           * @since 3.2.3
           */
          public class SystemProperties
          {
          
              /**
               * Thread-safe System.properties copy implementation.
               *
               * @see "https://issues.apache.org/jira/browse/MNG-5670"
               */
              public static void addSystemProperties( Properties props )
              {
                  props.putAll( getSystemProperties() );
              }
          
              /**
               * Returns System.properties copy.
               */
              public static Properties getSystemProperties()
              {
                  final Properties systemProperties = new Properties();
          
                  try ( final ByteArrayOutputStream out = new ByteArrayOutputStream() )
                  {
                      System.getProperties().store( out, null );
          
                      try ( final ByteArrayInputStream in = new ByteArrayInputStream( out.toByteArray() ) )
                      {
                          systemProperties.load( in );
                      }
                  }
                  catch ( final IOException e )
                  {
                      throw new AssertionError( "Unexpected IO error copying system properties.", e );
                  }
          
                  return systemProperties;
              }
          
          }
          
          Show
          schulte77 Christian Schulte added a comment - I'd do it that way: package org.apache.maven.properties.internal; /* * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file * distributed with this work for additional information * regarding copyright ownership. The ASF licenses this file * to you under the Apache License, Version 2.0 (the * "License" ); you may not use this file except in compliance * with the License. You may obtain a copy of the License at * * http: //www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, * software distributed under the License is distributed on an * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY * KIND, either express or implied. See the License for the * specific language governing permissions and limitations * under the License. */ import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; import java.io.IOException; import java.util.Properties; /** * @since 3.2.3 */ public class SystemProperties { /** * Thread -safe System .properties copy implementation. * * @see "https: //issues.apache.org/jira/browse/MNG-5670" */ public static void addSystemProperties( Properties props ) { props.putAll( getSystemProperties() ); } /** * Returns System .properties copy. */ public static Properties getSystemProperties() { final Properties systemProperties = new Properties(); try ( final ByteArrayOutputStream out = new ByteArrayOutputStream() ) { System .getProperties().store( out, null ); try ( final ByteArrayInputStream in = new ByteArrayInputStream( out.toByteArray() ) ) { systemProperties.load( in ); } } catch ( final IOException e ) { throw new AssertionError( "Unexpected IO error copying system properties." , e ); } return systemProperties; } }
          Hide
          michael-o Michael Osipov added a comment - - edited

          Very interesting approach. Guillaume Boué, what do you think?

          Show
          michael-o Michael Osipov added a comment - - edited Very interesting approach. Guillaume Boué , what do you think?
          Hide
          schulte77 Christian Schulte added a comment -

          We've had a similar problem in the 'maven-aether-provider'. See lines 133ff of class MavenRepositorySystemUtils.java. That's the only way to create a copy of the system properties in a thread-safe way. It should be done the same way here.

          Show
          schulte77 Christian Schulte added a comment - We've had a similar problem in the 'maven-aether-provider'. See lines 133ff of class MavenRepositorySystemUtils.java . That's the only way to create a copy of the system properties in a thread-safe way. It should be done the same way here.
          Hide
          famod Falko Modler added a comment -

          I still don't know what is removing system properties. There is a similar problem with cxf-codegen: CXF-7083
          We do set system properties in tests but that is happening in a forked JVM (started by surefire-plugin).

          Show
          famod Falko Modler added a comment - I still don't know what is removing system properties. There is a similar problem with cxf-codegen: CXF-7083 We do set system properties in tests but that is happening in a forked JVM (started by surefire-plugin).
          Hide
          michael-o Michael Osipov added a comment -

          Right, though I am confused who is actually removing properties from system and why causing this issue. All should be thread-safe if you operate on local properties instances.

          Show
          michael-o Michael Osipov added a comment - Right, though I am confused who is actually removing properties from system and why causing this issue. All should be thread-safe if you operate on local properties instances.
          Hide
          gboue Guillaume Boué added a comment -

          synchronized on the static method would lock the class, so you wouldn't be able to call getSystemProperties and addSystemProperties concurrently, which is safe to do now (since working on 2 different instances of Properties). You're right that it probably doesn't have any real impact though, but I think adding a null-check is simpler to understand and also covers all the possible scenarios.

          Show
          gboue Guillaume Boué added a comment - synchronized on the static method would lock the class, so you wouldn't be able to call getSystemProperties and addSystemProperties concurrently, which is safe to do now (since working on 2 different instances of Properties). You're right that it probably doesn't have any real impact though, but I think adding a null -check is simpler to understand and also covers all the possible scenarios.
          Hide
          michael-o Michael Osipov added a comment -

          Why not use synchronized to have an idiot-proof write to system properties? I hardly believe that this would introduce any huge performance penalty.

          Show
          michael-o Michael Osipov added a comment - Why not use synchronized to have an idiot-proof write to system properties? I hardly believe that this would introduce any huge performance penalty.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build maven-3.x #1394 (See https://builds.apache.org/job/maven-3.x/1394/)
          MNG-6105 properties.internal.SystemProperties.addSystemProperties() is (gboue: rev d8e3585e051db0293ddd41c781b39e0bf9ce350b)

          • (edit) maven-settings-builder/src/main/java/org/apache/maven/settings/building/DefaultSettingsBuildingRequest.java
          • (edit) maven-core/src/main/java/org/apache/maven/properties/internal/SystemProperties.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build maven-3.x #1394 (See https://builds.apache.org/job/maven-3.x/1394/ ) MNG-6105 properties.internal.SystemProperties.addSystemProperties() is (gboue: rev d8e3585e051db0293ddd41c781b39e0bf9ce350b) (edit) maven-settings-builder/src/main/java/org/apache/maven/settings/building/DefaultSettingsBuildingRequest.java (edit) maven-core/src/main/java/org/apache/maven/properties/internal/SystemProperties.java
          Hide
          gboue Guillaume Boué added a comment -

          I would prefer not to use clone() here, adding a null-check would be a good solution. Changes made in d8e3585e051db0293ddd41c781b39e0bf9ce350b. Once the snapshots are deployed, could you test if you still have the issue if you use the latest 3.4.0-SNAPSHOT of maven-core for the Assembly Plugin?

          Show
          gboue Guillaume Boué added a comment - I would prefer not to use clone() here, adding a null -check would be a good solution. Changes made in d8e3585e051db0293ddd41c781b39e0bf9ce350b . Once the snapshots are deployed, could you test if you still have the issue if you use the latest 3.4.0-SNAPSHOT of maven-core for the Assembly Plugin?

            People

            • Assignee:
              gboue Guillaume Boué
              Reporter:
              famod Falko Modler
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development