Introduce `EnvSetting` utility by armiol · Pull Request #1615 · SpineEventEngine/core-jvm

armiol · 2025-11-14T15:19:37Z

This changeset migrates the changes made to Spine 1.x on making EnvSetting utility available for public use.

Now, EnvSetting allows configuring the value that may differ per environment type:

    var setting = new EnvSetting<UserReader>();
    setting.use(projectsUserReader(), Production.class)
           .use(localUserReader(), Local.class)
           .use(testsUserReader(), Tests.class);
    
    // ...

    // The instance is returned according to the current `Environment` type.
    var reader = setting.value();

In the example above, there are different implementations of UserReader required in different usage scenarios. Previously, such a behaviour could only be achieved via numerous if ... else ... statements.

Another improvement made by this PR is the chaining of use(..) calls. Previously, when EnvSetting was an internal utility, use() methods were void.

Please note, EnvSetting is left non-thread-safe, since it is able to utulise Suppliers as the value providers. It makes it impossible to provide a reliable deadlock-free environment without overcomplicating things.

* Make the type `public`. * Allow chaining for `.use(..)` calls. * Make the access more thread-safe.

…ing the type of the lock.

Copilot

Pull Request Overview

This PR makes the EnvSetting utility class publicly available for configuring environment-specific values. It introduces method chaining for use() methods and adds thread-safety using StampedLock to handle concurrent read/write operations in multi-threaded scenarios.

Key changes:

Changed class visibility from package-private to public with chainable API methods
Added thread-safety with StampedLock for read/write synchronization
Introduced comprehensive thread-safety tests to validate concurrent access patterns

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
version.gradle.kts	Version bump to 2.0.0-SNAPSHOT.352
pom.xml	Version update to match Gradle version
dependencies.md	Automated dependency report regeneration with updated version and timestamp
server/src/main/java/io/spine/server/EnvSetting.java	Main implementation: made class public, added thread-safety with StampedLock, made methods chainable, updated documentation
server/src/test/java/io/spine/server/EnvSettingTest.java	Added comprehensive thread-safety tests and updated existing tests to use method chaining

Copilot · 2025-11-14T17:05:35Z

server/src/main/java/io/spine/server/EnvSetting.java


    /**
-     * Represents an operation over the setting that returns no result and may finish with an error.
+     * Executes the provided read operation under the write lock on the value.


The javadoc comment incorrectly states "Executes the provided read operation under the write lock" when it should say "Executes the provided write operation under the write lock".

Suggested change

* Executes the provided read operation under the write lock on the value.

* Executes the provided write operation under the write lock on the value.

Copilot · 2025-11-14T17:05:35Z

server/src/main/java/io/spine/server/EnvSetting.java

+ *         new EnvSetting<>(Tests.class, () -> fallbackStorageFactory);
 *
 *     // `use` was never called, so the fallback value is calculated and returned.
 *     assertThat(setting.optionalValue()).isPresent();


The code example in the documentation uses optionalValue() without providing an environment type argument (line 69), but this method requires a Class<? extends EnvironmentType<?>> parameter according to the method signature.

Either correct the example to pass the environment type (e.g., setting.optionalValue(Tests.class)), or if there's an overload without parameters, the example needs clarification.

Suggested change

* assertThat(setting.optionalValue()).isPresent();

* assertThat(setting.optionalValue(Tests.class)).isPresent();

Copilot · 2025-11-14T17:05:36Z

server/src/main/java/io/spine/server/EnvSetting.java

+     * <p>If for the current environment, there is no value set in this setting
+     * return a fallback value. If no fallback was configured,


Grammar issue: "return a fallback value" should be "returns a fallback value" to match the subject-verb agreement in the sentence.

Suggested change

* <p>If for the current environment, there is no value set in this setting

* return a fallback value. If no fallback was configured,

* <p>If for the current environment, there is no value set in this setting,

* returns a fallback value. If no fallback was configured,

Copilot · 2025-11-14T17:05:36Z

server/src/main/java/io/spine/server/EnvSetting.java

+        if (value == null) {
+            return Optional.empty();
+        }
+        return Optional.of(value.value);


The valueFor(Supplier<Class<...>>) method directly accesses the value field of the Value object (line 209) outside of any synchronization, which could lead to a race condition. The Value class uses synchronization in its get() and isResolved() methods, but this code bypasses those methods.

Replace value.value with value.get() to ensure thread-safe access to the lazily-initialized value.

Suggested change

return Optional.of(value.value);

return Optional.of(value.get());

server/src/main/java/io/spine/server/EnvSetting.java

Copilot · 2025-11-14T17:27:18Z

@armiol I've opened a new pull request, #1616, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot

Pull Request Overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

server/src/main/java/io/spine/server/EnvSetting.java

# Conflicts: # dependencies.md

Copilot

Pull Request Overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Copilot · 2025-11-14T19:26:14Z

server/src/main/java/io/spine/server/EnvSetting.java

+        var value = readWithLock(() -> {
+            var envType = type.get();
+            checkNotNull(envType);
+            return this.environmentValues.get(envType);
+        });
+        if (value == null) {
+            return Optional.empty();
+        }
+        return Optional.of(value.get());


Potential lock ordering issue with Value.get() call outside read lock

After acquiring the Value object under a read lock (lines 234-238), the value.get() method is called outside of any lock (line 242). The Value.get() method internally uses synchronization, which creates a potential for inconsistent lock ordering. Additionally, if the lazy supplier takes a long time to execute, this could lead to unexpected behavior.

Suggested fix:
Either call value.get() within the read lock, or reconsider if the Value class needs its own synchronization given that all access to the map is already protected by StampedLock.

Suggested change

var value = readWithLock(() -> {

var envType = type.get();

checkNotNull(envType);

return this.environmentValues.get(envType);

});

if (value == null) {

return Optional.empty();

}

return Optional.of(value.get());

V result = readWithLock(() -> {

var envType = type.get();

checkNotNull(envType);

var value = this.environmentValues.get(envType);

return (value != null) ? value.get() : null;

});

return (result != null) ? Optional.of(result) : Optional.empty();

Copilot · 2025-11-14T19:26:14Z

server/src/main/java/io/spine/server/EnvSetting.java

 /**
 * A mutable value that may differ between {@linkplain EnvironmentType environment types}.
 *
 * <p>For example:
- * <pre>
- * {@literal EnvSetting<StorageFactory>} storageFactory ={@literal new EnvSetting<>();}
- * storageFactory.use(InMemoryStorageFactory.newInstance(), Production.class);
+ * <pre>{@code
+ *     EnvSetting<StorageFactory> storageFactory = new EnvSetting<>();
+ *     storageFactory.use(InMemoryStorageFactory.newInstance(), Production.class)
+ *                   .use(new MemoizingStorageFactory(), Tests.class);
 *
- * assertThat(storageFactory.optionalValue(Production.class)).isPresent();
- * assertThat(storageFactory.optionalValue(Tests.class)).isEmpty();
- * </pre>
+ *     // Provides the `StorageFactory` for the current environment of the application.
+ *     StorageFactory currentStorageFactory = storageFactory.value();
+ * }</pre>
 *
 * <h2>Fallback</h2>
 * <p>{@code EnvSetting} allows to configure a default value for an environment type. It is used
 * when the value for the environment hasn't been {@linkplain #use(Object, Class) set explicitly}.
- * <pre>
+ * <pre>{@code
 *      // Assuming the environment is `Tests`.
- *
- *      StorageFactory fallbackStorageFactory = createStorageFactory();
- *     {@literal EnvSetting<StorageFactory>} setting =
- *          {@literal new EnvSetting<>(Tests.class, () -> fallbackStorageFactory)};
+ *     StorageFactory fallbackStorageFactory = createStorageFactory();
+ *     EnvSetting<StorageFactory> setting =
+ *         new EnvSetting<>(Tests.class, () -> fallbackStorageFactory);
 *
 *     // `use` was never called, so the fallback value is calculated and returned.
- *     assertThat(setting.optionalValue()).isPresent();
+ *     assertThat(setting.optionalValue(Tests.class)).isPresent();
 *     assertThat(setting.value()).isSameInstanceAs(fallbackStorageFactory);
- * </pre>
+ * }</pre>
 *
 * <p>Fallback values are calculated once on first {@linkplain #value(Class) access} for the
 * specified environment. Every subsequent access returns the cached value.
- *
- * <pre>
+ * <pre>{@code
 *      // This `Supplier` is calculated only once.
- *     {@literal Supplier<StorageFactory>} fallbackStorage = InMemoryStorageFactory::newInstance;
- *
- *     {@literal EnvSetting<StorageFactory>} setting =
- *     {@literal new EnvSetting<>(Tests.class, fallbackStorage);}
+ *     Supplier<StorageFactory> fallbackStorage = InMemoryStorageFactory::newInstance;
+ *     EnvSetting<StorageFactory> setting = new EnvSetting<>(Tests.class, fallbackStorage);
 *
 *     // `Supplier` is calculated and cached.
 *     StorageFactory storageFactory = setting.value();
 *
 *     // Fallback value is taken from cache.
 *     StorageFactory theSameFactory = setting.value();
- * </pre>
- *
- * <p>{@code EnvSetting} values do not determine the environment themselves: it's up to the
- * caller to ask for the appropriate one.
+ * }</pre>


[nitpick] Inconsistent documentation style

The documentation example uses {@literal} in some places (line 51) but not in others (line 52-53, 56, 65-66, 77-78). For consistency and better rendering, all generic type parameters in code examples should be handled uniformly.

Suggested fix:
Either use {@code} blocks for all examples (as done on lines 50-57) or consistently apply {@literal} where needed. The {@code} approach is simpler and more readable.

Copilot · 2025-11-14T19:26:15Z

server/src/test/java/io/spine/server/EnvSettingTest.java

+        @Test
+        @DisplayName("allowing multiple threads to read simultaneously " +
+                "but postpone concurrent write operations")
+        void testReadOperations() {
+            var initialValue = randomUUID();
+            setting.use(initialValue, Local.class);
+
+            var readBlockingFuture = runBlockingReadOperation(Local.class, initialValue);
+            sleepUninterruptibly(100, MILLISECONDS);
+
+            var actualValue = setting.value(Local.class);
+            assertThat(actualValue).isEqualTo(initialValue);
+
+            // This "write" operation should be waiting until the lock is released
+            // via `latch.countDown()`.
+            var rewrittenValue = randomUUID();
+            readWriteExecutors.submit(() -> {
+                setting.use(rewrittenValue, Local.class);
+            });
+            sleepUninterruptibly(100, MILLISECONDS);
+
+            var stillSameValue = setting.value(Local.class);
+            assertThat(stillSameValue).isEqualTo(initialValue);
+
+            latch.countDown();
+            await(readBlockingFuture);
+
+            var newValue = setting.value(Local.class);
+            assertThat(newValue).isEqualTo(rewrittenValue);
+        }
+
+        @Test
+        @DisplayName("allowing a write operation to hold exclusive access, " +
+                "blocking concurrent reads and writes until complete")
+        void testWriteOperations() {
+            var initialValue = randomUUID();
+            var writeBlockingFuture = runBlockingWriteOperation(Local.class, initialValue);
+            sleepUninterruptibly(100, MILLISECONDS);
+
+            var rewrittenValue = randomUUID();
+            var writeFuture =
+                    runVerifyingWriteOperation(Local.class, rewrittenValue, initialValue);
+            sleepUninterruptibly(100, MILLISECONDS);


Hard-coded sleep times reduce test reliability

The test uses hard-coded sleep times (100ms on lines 281, 292, 310, 315) to ensure certain thread states. This approach is brittle and can lead to flaky tests on slower systems or under high load. The sleeps don't provide any guarantee that the submitted tasks have actually started execution or reached the blocking point.

Suggested fix:
Use proper synchronization primitives (e.g., additional CountDownLatch instances) to signal when threads have reached specific execution points, rather than relying on timing assumptions:

// Signal when the blocking operation has started var operationStarted = new CountDownLatch(1); var future = readWriteExecutors.submit(() -> { operationStarted.countDown(); awaitUninterruptibly(latch); // ... rest of operation }); awaitUninterruptibly(operationStarted); // Now we know the operation has started and is blocked

Copilot

Pull Request Overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Copilot · 2025-11-15T18:40:25Z

server/src/main/java/io/spine/server/EnvSetting.java

@@ -149,14 +160,22 @@ void ifPresentForEnvironment(Class<? extends EnvironmentType<?>> type,
     * <p>This means the operation is applied to all passed setting {@linkplain #environmentValues


[nitpick] The documentation wording "all passed setting values" is unclear. Consider rewording to "all configured values" or "all values that have been set" for better clarity, since "passed" might be confused with method parameters.

Suggested change

* <p>This means the operation is applied to all passed setting {@linkplain #environmentValues

* <p>This means the operation is applied to all configured {@linkplain #environmentValues

Copilot · 2025-11-15T18:40:26Z

server/src/main/java/io/spine/server/EnvSetting.java

+         * Performs this operation on the specified value.
+         *
+         * @param value
+         *         the value to use in this operation


The accept method declares throws Exception but doesn't document it with @throws. Consider adding:

/** * Performs this operation on the specified value. * * @param value * the value to use in this operation * @throws Exception * if the operation fails */

Suggested change

* the value to use in this operation

* the value to use in this operation

* @throws Exception

* if the operation fails

Copilot · 2025-11-15T18:40:26Z

server/src/main/java/io/spine/server/EnvSetting.java

+            new ConcurrentHashMap<>();

    private final Map<Class<? extends EnvironmentType<?>>, Supplier<V>> fallbacks =
-            new HashMap<>();
+            new ConcurrentHashMap<>();


The PR description states that EnvSetting is "left non-thread-safe" as a deliberate choice, but the implementation uses ConcurrentHashMap (lines 92, 95). This creates confusion about the actual thread-safety guarantees.

Either:

Use regular HashMap to match the documented non-thread-safe contract, or

Update the documentation to clarify the partial thread-safety provided by ConcurrentHashMap (e.g., "This type provides limited thread-safety for read operations but is not safe for concurrent modifications")

Copilot

Pull Request Overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

server/src/main/java/io/spine/server/EnvSetting.java

Copilot

Pull Request Overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

armiol · 2025-11-15T19:02:24Z

@alexander-yevsyukov PTAL. That's NOT urgent by any means, given how much time we've spent having fun with copilot.

alexander-yevsyukov

LGTM

armiol added 3 commits November 14, 2025 14:28

Migrate #1550 to 2.x.

214f896

* Make the type `public`. * Allow chaining for `.use(..)` calls. * Make the access more thread-safe.

Bump version -> 2.0.0-SNAPSHOT.352

b7b08af

Add a missing empty line.

7596bf4

armiol self-assigned this Nov 14, 2025

armiol added 4 commits November 14, 2025 15:20

Update the reports.

bac08c7

Add the copyright header previously removed by mistake.

e68b9fe

Improve the docs.

24f9b4f

Improve the test, discover a deadlock-producing bug, address by chang…

3a5e41d

…ing the type of the lock.

armiol marked this pull request as ready for review November 14, 2025 16:58

armiol requested a review from Copilot November 14, 2025 16:58

armiol added this to the M1 milestone Nov 14, 2025

armiol added this to v2.0 Nov 14, 2025

Copilot started reviewing on behalf of armiol November 14, 2025 16:59 View session

Copilot finished reviewing on behalf of armiol November 14, 2025 17:01

Copilot AI reviewed Nov 14, 2025

View reviewed changes

armiol added 4 commits November 14, 2025 17:20

Use the correct verb.

e95c39b

Correct the documentation.

2930e0a

Fix the grammar.

7b05084

Return the value via its supplier.

4ec9abb

Copilot AI mentioned this pull request Nov 14, 2025

[WIP] Revise EnvSetting utility based on review feedback #1616

Closed

armiol requested a review from Copilot November 14, 2025 17:28

Copilot started reviewing on behalf of armiol November 14, 2025 17:29 View session

Copilot finished reviewing on behalf of armiol November 14, 2025 17:33

Copilot AI reviewed Nov 14, 2025

View reviewed changes

armiol added 2 commits November 14, 2025 17:41

Mention the tread safety in the documentation.

cfc9df9

Avoid unnecessary synchronization in ctor.

06bc73b

armiol requested a review from Copilot November 14, 2025 17:43

Copilot started reviewing on behalf of armiol November 14, 2025 17:44 View session

Copilot finished reviewing on behalf of armiol November 14, 2025 17:46

armiol added 4 commits November 14, 2025 19:08

Remove unused import.

0a7ec69

Merge branch 'master' into expose-env-settings-2x

a071460

# Conflicts: # dependencies.md

Bump version -> 2.0.0-SNAPSHOT.353

5cb6870

Update the reports.

6dfd242

armiol requested a review from Copilot November 14, 2025 19:18

Copilot started reviewing on behalf of armiol November 14, 2025 19:18 View session

Copilot finished reviewing on behalf of armiol November 14, 2025 19:21

Copilot AI reviewed Nov 14, 2025

View reviewed changes

Simplify EnvSetting and leave it non-thread-safe.

887ab62

armiol requested a review from Copilot November 15, 2025 18:34

Copilot started reviewing on behalf of armiol November 15, 2025 18:34 View session

Copilot finished reviewing on behalf of armiol November 15, 2025 18:36

Copilot AI reviewed Nov 15, 2025

View reviewed changes

armiol added 3 commits November 15, 2025 18:43

Revert to a regular HashMap.

1bc7b40

Clarify the wording.

5e5e725

Document the @throws clause.

6d44c8d

armiol requested a review from Copilot November 15, 2025 18:45

Copilot started reviewing on behalf of armiol November 15, 2025 18:45 View session

Copilot finished reviewing on behalf of armiol November 15, 2025 18:45

Copilot AI reviewed Nov 15, 2025

View reviewed changes

server/src/main/java/io/spine/server/EnvSetting.java Show resolved Hide resolved

server/src/main/java/io/spine/server/EnvSetting.java Show resolved Hide resolved

armiol requested a review from Copilot November 15, 2025 18:53

Copilot started reviewing on behalf of armiol November 15, 2025 18:54 View session

Copilot finished reviewing on behalf of armiol November 15, 2025 18:56

Copilot AI reviewed Nov 15, 2025

View reviewed changes

armiol requested a review from alexander-yevsyukov November 15, 2025 19:01

alexander-yevsyukov approved these changes Nov 16, 2025

View reviewed changes

armiol merged commit 317d9d1 into master Nov 16, 2025
19 checks passed

armiol deleted the expose-env-settings-2x branch November 16, 2025 14:39

github-project-automation bot moved this to ✅ Done in v2.0 Nov 16, 2025

	* Executes the provided read operation under the write lock on the value.
	* Executes the provided write operation under the write lock on the value.

	* assertThat(setting.optionalValue()).isPresent();
	* assertThat(setting.optionalValue(Tests.class)).isPresent();

		* <p>If for the current environment, there is no value set in this setting
		* return a fallback value. If no fallback was configured,

	return Optional.of(value.value);
	return Optional.of(value.get());

		@@ -149,14 +160,22 @@ void ifPresentForEnvironment(Class<? extends EnvironmentType<?>> type,
		* <p>This means the operation is applied to all passed setting {@linkplain #environmentValues

Conversation

armiol commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI commented Nov 14, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

armiol commented Nov 15, 2025

Uh oh!

alexander-yevsyukov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

armiol commented Nov 14, 2025 •

edited

Loading