Skip to content

Commit e2731f6

Browse files
nosannosan
committed
Fix SingletonSupplier incorrectly handling null result
Previously, SingletonSupplier stored "null" in singletonInstance when the supplied instance was "null". On subsequent get() calls, this was treated as "uninitialized" and triggered another attempt to obtain an instance. This commit ensures that a "null" returned from the instanceSupplier or defaultSupplier is handled correctly, so that subsequent calls to get() return "null" consistently instead of repeatedly invoking the supplier. Signed-off-by: Dmytro Nosan <[email protected]>
1 parent 9e9d716 commit e2731f6

File tree

3 files changed

+201
-9
lines changed

3 files changed

+201
-9
lines changed

‎spring-core/src/main/java/org/springframework/util/function/SingletonSupplier.java

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ public class SingletonSupplier<T> implements Supplier<@Nullable T> {
4747

4848
private volatile @Nullable T singletonInstance;
4949

50+
private volatile boolean initialized;
5051
/**
5152
* Guards access to write operations on the {@code singletonInstance} field.
5253
*/
@@ -63,6 +64,7 @@ public SingletonSupplier(@Nullable T instance, Supplier<? extends @Nullable T> d
6364
this.instanceSupplier = null;
6465
this.defaultSupplier = defaultSupplier;
6566
this.singletonInstance = instance;
67+
this.initialized = (instance != null);
6668
}
6769

6870
/**
@@ -85,6 +87,7 @@ private SingletonSupplier(@Nullable T singletonInstance) {
8587
this.instanceSupplier = null;
8688
this.defaultSupplier = null;
8789
this.singletonInstance = singletonInstance;
90+
this.initialized = (singletonInstance != null);
8891
}
8992

9093

@@ -94,26 +97,24 @@ private SingletonSupplier(@Nullable T singletonInstance) {
9497
*/
9598
@Override
9699
public @Nullable T get() {
97-
T instance = this.singletonInstance;
98-
if (instance == null) {
100+
if (!this.initialized) {
99101
this.writeLock.lock();
100102
try {
101-
instance = this.singletonInstance;
102-
if (instance == null) {
103+
if (!this.initialized) {
103104
if (this.instanceSupplier != null) {
104-
instance = this.instanceSupplier.get();
105+
this.singletonInstance = this.instanceSupplier.get();
105106
}
106-
if (instance == null && this.defaultSupplier != null) {
107-
instance = this.defaultSupplier.get();
107+
if (this.singletonInstance == null && this.defaultSupplier != null) {
108+
this.singletonInstance = this.defaultSupplier.get();
108109
}
109-
this.singletonInstance = instance;
110+
this.initialized = true;
110111
}
111112
}
112113
finally {
113114
this.writeLock.unlock();
114115
}
115116
}
116-
return instance;
117+
return this.singletonInstance;
117118
}
118119

119120
/**

‎spring-core/src/test/java/org/springframework/util/function/SingletonSupplierTests.java

Lines changed: 190 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,190 @@
1+
/*
2+
* Copyright 2002-present the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package org.springframework.util.function;
18+
19+
import java.util.ArrayList;
20+
import java.util.List;
21+
import java.util.concurrent.CountDownLatch;
22+
import java.util.concurrent.ExecutorService;
23+
import java.util.concurrent.Executors;
24+
import java.util.concurrent.Future;
25+
import java.util.concurrent.atomic.AtomicInteger;
26+
import java.util.function.Supplier;
27+
28+
import org.junit.jupiter.api.RepeatedTest;
29+
import org.junit.jupiter.api.Test;
30+
31+
import static org.assertj.core.api.Assertions.assertThat;
32+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
33+
34+
/**
35+
* Tests for {@link SingletonSupplier}.
36+
*
37+
* @author Dmytro Nosan
38+
*/
39+
class SingletonSupplierTests {
40+
41+
@Test
42+
void shouldReturnDefaultWhenInstanceSupplierReturnsNull() {
43+
SingletonSupplier<String> singletonSupplier = new SingletonSupplier<>(() -> null, () -> "Default");
44+
assertThat(singletonSupplier.get()).isEqualTo("Default");
45+
}
46+
47+
@Test
48+
void shouldReturnNullForOfNullableWithNullInstance() {
49+
SingletonSupplier<String> singletonSupplier = SingletonSupplier.ofNullable((String) null);
50+
assertThat(singletonSupplier).isNull();
51+
}
52+
53+
@Test
54+
void shouldReturnNullForOfNullableWithNullSupplier() {
55+
SingletonSupplier<String> singletonSupplier = SingletonSupplier.ofNullable((Supplier<String>) null);
56+
assertThat(singletonSupplier).isNull();
57+
}
58+
59+
@Test
60+
void shouldReturnNullWhenAllSuppliersReturnNull() {
61+
SingletonSupplier<String> singletonSupplier = new SingletonSupplier<>(() -> null, () -> null);
62+
assertThat(singletonSupplier.get()).isNull();
63+
}
64+
65+
@Test
66+
void shouldReturnNullWhenNoInstanceOrDefaultSupplier() {
67+
SingletonSupplier<String> singletonSupplier = new SingletonSupplier<>((String) null, null);
68+
assertThat(singletonSupplier.get()).isNull();
69+
}
70+
71+
@Test
72+
void shouldReturnSingletonInstanceOnMultipleCalls() {
73+
SingletonSupplier<String> singletonSupplier = SingletonSupplier.of("Hello");
74+
assertThat(singletonSupplier.get()).isEqualTo("Hello");
75+
assertThat(singletonSupplier.get()).isEqualTo("Hello");
76+
}
77+
78+
79+
@Test
80+
void shouldReturnSingletonInstanceOnMultipleSupplierCalls() {
81+
SingletonSupplier<String> singletonSupplier = SingletonSupplier.of(new HelloStringSupplier());
82+
assertThat(singletonSupplier.get()).isEqualTo("Hello 0");
83+
assertThat(singletonSupplier.get()).isEqualTo("Hello 0");
84+
}
85+
86+
@Test
87+
void shouldReturnSupplierForOfNullableWithNonNullInstance() {
88+
SingletonSupplier<String> singletonSupplier = SingletonSupplier.ofNullable("Hello");
89+
assertThat(singletonSupplier).isNotNull();
90+
assertThat(singletonSupplier.get()).isEqualTo("Hello");
91+
}
92+
93+
@Test
94+
void shouldReturnSupplierForOfNullableWithNonNullSupplier() {
95+
SingletonSupplier<String> singletonSupplier = SingletonSupplier.ofNullable(() -> "Hello");
96+
assertThat(singletonSupplier).isNotNull();
97+
assertThat(singletonSupplier.get()).isEqualTo("Hello");
98+
}
99+
100+
@Test
101+
void shouldThrowWhenObtainCalledAndNoInstanceAvailable() {
102+
SingletonSupplier<String> singletonSupplier = new SingletonSupplier<>((String) null, null);
103+
assertThatThrownBy(singletonSupplier::obtain).isInstanceOf(IllegalStateException.class)
104+
.hasMessage("No instance from Supplier");
105+
}
106+
107+
@Test
108+
void shouldUseDefaultSupplierWhenInstanceIsNull() {
109+
SingletonSupplier<String> singletonSupplier = new SingletonSupplier<>((String) null, () -> "defaultSupplier");
110+
assertThat(singletonSupplier.get()).isEqualTo("defaultSupplier");
111+
}
112+
113+
@Test
114+
void shouldUseDefaultSupplierWhenInstanceSupplierReturnsNull() {
115+
SingletonSupplier<String> singletonSupplier = new SingletonSupplier<>((Supplier<String>) null, () -> "defaultSupplier");
116+
assertThat(singletonSupplier.get()).isEqualTo("defaultSupplier");
117+
}
118+
119+
@Test
120+
void shouldUseInstanceSupplierWhenProvidedAndIgnoreDefaultSupplier() {
121+
AtomicInteger defaultValue = new AtomicInteger();
122+
SingletonSupplier<Integer> singletonSupplier = new SingletonSupplier<>(() -> -1, defaultValue::incrementAndGet);
123+
assertThat(singletonSupplier.get()).isEqualTo(-1);
124+
assertThat(defaultValue.get()).isEqualTo(0);
125+
}
126+
127+
@Test
128+
void shouldUseInstanceWhenProvidedAndIgnoreDefaultSupplier() {
129+
AtomicInteger defaultValue = new AtomicInteger();
130+
SingletonSupplier<Integer> singletonSupplier = new SingletonSupplier<>(-1, defaultValue::incrementAndGet);
131+
assertThat(singletonSupplier.get()).isEqualTo(-1);
132+
assertThat(defaultValue.get()).isEqualTo(0);
133+
}
134+
135+
@Test
136+
void shouldReturnConsistentlyNullSingletonInstanceOnMultipleSupplierCalls() {
137+
SingletonSupplier<String> singletonSupplier = SingletonSupplier.of(new Supplier<>() {
138+
139+
int count = 0;
140+
141+
@Override
142+
public String get() {
143+
if (this.count++ == 0) {
144+
return null;
145+
}
146+
return "Hello";
147+
}
148+
});
149+
150+
assertThat(singletonSupplier.get()).isNull();
151+
assertThat(singletonSupplier.get()).isNull();
152+
}
153+
154+
@RepeatedTest(100)
155+
void shouldReturnSingletonInstanceOnMultipleConcurrentSupplierCalls() throws Exception {
156+
int numberOfThreads = 4;
157+
CountDownLatch ready = new CountDownLatch(numberOfThreads);
158+
CountDownLatch start = new CountDownLatch(1);
159+
List<Future<String>> futures = new ArrayList<>();
160+
SingletonSupplier<String> singletonSupplier = SingletonSupplier.of(new HelloStringSupplier());
161+
ExecutorService executorService = Executors.newFixedThreadPool(numberOfThreads);
162+
try {
163+
for (int i = 0; i < numberOfThreads; i++) {
164+
futures.add(executorService.submit(() -> {
165+
ready.countDown();
166+
start.await();
167+
return singletonSupplier.obtain();
168+
}));
169+
}
170+
ready.await();
171+
start.countDown();
172+
assertThat(futures).extracting(Future::get).containsOnly("Hello 0");
173+
}
174+
finally {
175+
executorService.shutdown();
176+
}
177+
}
178+
179+
180+
private static final class HelloStringSupplier implements Supplier<String> {
181+
182+
private final AtomicInteger count = new AtomicInteger();
183+
184+
@Override
185+
public String get() {
186+
return "Hello " + this.count.getAndIncrement();
187+
}
188+
}
189+
190+
}

‎spring-jdbc/src/test/java/org/springframework/jdbc/support/SQLErrorCodeSQLExceptionTranslatorTests.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,7 @@ void dataSourceInitialization() throws Exception {
209209

210210
reset(dataSource);
211211
given(dataSource.getConnection()).willReturn(connection);
212+
translator = new SQLErrorCodeSQLExceptionTranslator(dataSource);
212213
assertThat(translator.translate("test", null, duplicateKeyException))
213214
.isInstanceOf(DuplicateKeyException.class);
214215

0 commit comments

Comments
 (0)