Skip to content

Commit a722667

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 a722667

File tree

3 files changed

+208
-1
lines changed

3 files changed

+208
-1
lines changed

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

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import java.util.concurrent.locks.ReentrantLock;
2121
import java.util.function.Supplier;
2222

23+
import org.jspecify.annotations.NonNull;
2324
import org.jspecify.annotations.Nullable;
2425

2526
import org.springframework.lang.Contract;
@@ -41,6 +42,15 @@
4142
*/
4243
public class SingletonSupplier<T> implements Supplier<@Nullable T> {
4344

45+
private static final Object NULL = new Object() {
46+
47+
@Override
48+
public String toString() {
49+
return "null";
50+
}
51+
52+
};
53+
4454
private final @Nullable Supplier<? extends @Nullable T> instanceSupplier;
4555

4656
private final @Nullable Supplier<? extends @Nullable T> defaultSupplier;
@@ -106,14 +116,15 @@ private SingletonSupplier(@Nullable T singletonInstance) {
106116
if (instance == null && this.defaultSupplier != null) {
107117
instance = this.defaultSupplier.get();
108118
}
119+
instance = (instance != null ? instance : asNull());
109120
this.singletonInstance = instance;
110121
}
111122
}
112123
finally {
113124
this.writeLock.unlock();
114125
}
115126
}
116-
return instance;
127+
return (instance != NULL ? instance : null);
117128
}
118129

119130
/**
@@ -166,4 +177,9 @@ public static <T> SingletonSupplier<T> of(Supplier<T> supplier) {
166177
return (supplier != null ? new SingletonSupplier<>(supplier) : null);
167178
}
168179

180+
@SuppressWarnings("unchecked")
181+
private @NonNull T asNull() {
182+
return (T) NULL;
183+
}
184+
169185
}

‎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<>(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 shouldUseInstanceSupplierWithDefaultSupplier() {
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 shouldUseInstanceWithDefaultSupplier() {
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)