systemProperties PropertySource in Environment implemented with Properties ,But Properties is synchronous, which will reduce performance !!!

Comment From: rstoyanchev

It's not very clear what you're talking about. When you say "synchronous" what alternative do you have in mind and what specific use cases are you concerned about? Also SystemEnvironmentPropertySource is based on a Map, not Properties.

Can you provide more context please?

Comment From: sbrannen

The system properties are actually stored in a PropertiesPropertySource which by default delegates to System.getProperties() as the source. That happens to be a synchronized Hashtable which implements Map.

So I guess the question is why AbstractEnvironment.getSystemProperties() delegates to System.getProperties() by default, when it could always create a ReadOnlySystemAttributesMap.

Of course, as you mentioned @rstoyanchev, we'd need to know when the synchronization of the system properties is problematic.

Comment From: spring-projects-issues

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

Comment From: brucelwl

@spring-issuemaster @sbrannen At present, I don't have a good solution. Maybe you can consider not putting systemProperties in the environment by default，But this may not be a good solution

Comment From: sbrannen

Maybe you can consider not putting systemProperties in the environment by default，But this may not be a good solution

No, that is unfortunately not an option since it would be a breaking change for a feature that thousands of projects rely on.

So I guess the question is why AbstractEnvironment.getSystemProperties() delegates to System.getProperties() by default, when it could always create a ReadOnlySystemAttributesMap.

@jhoeller, what do you think about switching to a read-only, non-synchronized map by default, potentially in 5.3?

Comment From: jhoeller

Even our ReadOnlySystemAttributesMap arrangement delegates to System.getProperty though, hitting the synchronized Properties instance underneath. If system properties are never supposed to be checked at all for performance reasons, maybe we need a setting along the lines of the existing "spring.getenv.ignore"... either for system properties specifically, or for the combination of system properties and system environment checks (since I suppose typically neither of those is needed then).

Comment From: sbrannen

Even our ReadOnlySystemAttributesMap arrangement delegates to System.getProperty though, hitting the synchronized Properties instance underneath.

Indeed. I was thinking ReadOnlySystemAttributesMap made an upfront copy. Mea culpa.

If system properties are never supposed to be checked at all for performance reasons, maybe we need a setting along the lines of the existing "spring.getenv.ignore"... either for system properties specifically, or for the combination of system properties and system environment checks (since I suppose typically neither of those is needed then).

I agree. Something like spring.env.system.properties.cache=true could signal that JVM system properties should be retrieved once and cached in a read-only map. And the same would hold for spring.env.environment.variables.cache=true (though naming is always a challenge).

Comment From: jhoeller

For a start, it seems straightforward to build a custom Environment subclass where the "systemProperties" source is being replaced with a copy, along the lines of StandardEnvironment which uses the same common customizePropertySources hook for registering the default "systemProperties" source.

However, that should not be necessary on a modern JVM: As of JDK 9 build 120, java.util.Properties uses an internal ConcurrentHashMap instead of the inherited Hashtable storage. This apparently came from a ClassLoader deadlock: https://bugs.openjdk.org/browse/JDK-8029891 https://github.com/openjdk/jdk/commit/0260528ef92c4cdfd14198639b78e8e6d0c81613

From that perspective, we are not going to introduce any workaround on our end here.