Skip to content

Commit 4e82a4a

Browse files
authored
feat: throw IllegalStateException when Signal.get() is called outside a reactive context (#23580)
Signal.get() now throws when called outside a reactive context (effect, computed signal, untracked block, or transaction) to prevent accidental non-reactive reads like `new Span("Name: " + nameSignal.get())` that silently return the value but never update. Users should use peek() for one-time reads. Key changes: - UsageTracker: add DELIBERATELY_UNTRACKED sentinel so untracked() sets a sentinel instead of removing the tracker, distinguishing "deliberately untracked" from "no tracking context" - AbstractLocalSignal/AbstractSignal: guard get() with isGetAllowed() check (shared signals also allow get() inside transactions) - ComputedSignal: remove peek() override so computed signals can be read outside reactive contexts via inherited AbstractSignal.peek() - Production code: use peek() where get() was called outside reactive contexts (Html, VaadinSession, SharedMapSignal.toString, Binder) Fixes #22123
1 parent 77f22cf commit 4e82a4a

31 files changed

+452
-305
lines changed

flow-data/src/test/java/com/vaadin/flow/data/binder/BinderSignalTest.java

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -717,7 +717,7 @@ public void getValidationStatus_signalInitialized() {
717717
.getValidationStatus();
718718
assertNotNull(statusSignal,
719719
"getValidationStatus() should setup validation status signal");
720-
assertNotNull(statusSignal.get(),
720+
assertNotNull(statusSignal.peek(),
721721
"validation status signal value should not be null initially");
722722
}
723723

@@ -741,18 +741,18 @@ public void getValidationStatus_statusChangeUpdatesSignal() {
741741
.withValidator(value -> !value.isEmpty(), "").bind("lastName");
742742
binder.setBean(item);
743743

744-
assertTrue(binder.getValidationStatus().get().isOk());
744+
assertTrue(binder.getValidationStatus().peek().isOk());
745745

746746
firstNameField.setValue("");
747-
assertFalse(binder.getValidationStatus().get().isOk());
747+
assertFalse(binder.getValidationStatus().peek().isOk());
748748
firstNameField.setValue("foo");
749-
assertTrue(binder.getValidationStatus().get().isOk());
749+
assertTrue(binder.getValidationStatus().peek().isOk());
750750
lastNameField.setValue("");
751-
assertFalse(binder.getValidationStatus().get().isOk());
751+
assertFalse(binder.getValidationStatus().peek().isOk());
752752
firstNameField.setValue("");
753-
assertFalse(binder.getValidationStatus().get().isOk());
753+
assertFalse(binder.getValidationStatus().peek().isOk());
754754
firstNameField.setValue("foo");
755-
assertFalse(binder.getValidationStatus().get().isOk());
755+
assertFalse(binder.getValidationStatus().peek().isOk());
756756
}
757757

758758
// verifies that field-specific validation statuses are updated correctly
@@ -769,18 +769,18 @@ public void getValidationStatus_fieldChanged_validationStatusSignalUpdated() {
769769
binder.setBean(item);
770770

771771
assertTrue(
772-
binder.getValidationStatus().get().getFieldValidationStatuses()
772+
binder.getValidationStatus().peek().getFieldValidationStatuses()
773773
.stream().noneMatch(BindingValidationStatus::isError));
774-
assertTrue(binder.getValidationStatus().get().isOk());
774+
assertTrue(binder.getValidationStatus().peek().isOk());
775775

776776
lastNameField.setValue(""); // change to invalid state
777777

778-
assertFalse(binder.getValidationStatus().get().isOk());
779-
var firstNameValidationStatuses = binder.getValidationStatus().get()
778+
assertFalse(binder.getValidationStatus().peek().isOk());
779+
var firstNameValidationStatuses = binder.getValidationStatus().peek()
780780
.getFieldValidationStatuses().stream()
781781
.filter(status -> status.getBinding() == firstNameBinding)
782782
.toList();
783-
var otherValidationStatuses = binder.getValidationStatus().get()
783+
var otherValidationStatuses = binder.getValidationStatus().peek()
784784
.getFieldValidationStatuses().stream()
785785
.filter(status -> status.getBinding() != firstNameBinding)
786786
.toList();
@@ -794,7 +794,7 @@ public void getValidationStatus_fieldChanged_validationStatusSignalUpdated() {
794794
"Expected first name field to have an error");
795795

796796
lastNameField.setValue("Smith");
797-
assertTrue(binder.getValidationStatus().get().isOk());
797+
assertTrue(binder.getValidationStatus().peek().isOk());
798798
}
799799

800800
@Test

flow-server/src/main/java/com/vaadin/flow/component/Html.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ public Html(Signal<String> htmlSignal) {
139139
if (htmlSignal == null) {
140140
throw new IllegalArgumentException("HTML signal cannot be null");
141141
}
142-
String outerHtml = htmlSignal.get();
142+
String outerHtml = htmlSignal.peek();
143143
if (outerHtml == null || outerHtml.isEmpty()) {
144144
throw new IllegalArgumentException("HTML cannot be null or empty");
145145
}

flow-server/src/main/java/com/vaadin/flow/server/VaadinSession.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1167,7 +1167,7 @@ private void writeObject(java.io.ObjectOutputStream stream)
11671167
}
11681168

11691169
// Sync locale field from signal for serialization
1170-
locale = localeSignal.get();
1170+
locale = localeSignal.peek();
11711171
stream.defaultWriteObject();
11721172
if (serializeUIs) {
11731173
stream.writeObject(uIs);

flow-server/src/main/java/com/vaadin/flow/signals/Signal.java

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,8 +69,17 @@ public interface Signal<T> extends Serializable {
6969
* Reading the value inside an {@link #unboundEffect(EffectAction)} or
7070
* {@link #computed(SignalComputation)} callback sets up that effect or
7171
* computed signal to depend on the signal.
72+
* <p>
73+
* This method must only be called within a reactive context such as an
74+
* effect, a computed signal, an explicit {@link #untracked(ValueSupplier)}
75+
* block, or a {@link #runInTransaction(TransactionTask) transaction}.
76+
* Calling it outside such a context throws an
77+
* {@link IllegalStateException}. Use {@link #peek()} for one-time reads
78+
* that do not need dependency tracking.
7279
*
7380
* @return the signal value
81+
* @throws IllegalStateException
82+
* if called outside a reactive context
7483
*/
7584
@Nullable
7685
T get();
@@ -79,6 +88,10 @@ public interface Signal<T> extends Serializable {
7988
* Reads the value without setting up any dependencies. This method returns
8089
* the same value as {@link #get()} but without creating a dependency when
8190
* used inside a transaction, effect or computed signal.
91+
* <p>
92+
* Unlike {@link #get()}, this method can be called outside a reactive
93+
* context and is the recommended way to read a signal value for one-time
94+
* use, such as logging, assertions, or initializing non-reactive UI.
8295
*
8396
* @return the signal value
8497
*/

flow-server/src/main/java/com/vaadin/flow/signals/impl/ComputedSignal.java

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -273,12 +273,6 @@ public T peekConfirmed() {
273273
"Cannot peek a computed signal");
274274
}
275275

276-
@Override
277-
public T peek() {
278-
throw new UnsupportedOperationException(
279-
"Cannot peek a computed signal");
280-
}
281-
282276
@Override
283277
public SharedNodeSignal asNode() {
284278
throw new UnsupportedOperationException(

flow-server/src/main/java/com/vaadin/flow/signals/impl/UsageTracker.java

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,13 @@ public Registration onNextChange(TransientListener listener) {
165165
}
166166
};
167167

168+
/**
169+
* Sentinel registrar that represents a deliberately untracked context,
170+
* distinguishing it from having no tracking context at all.
171+
*/
172+
private static final UsageRegistrar DELIBERATELY_UNTRACKED = usage -> {
173+
};
174+
168175
private static final ThreadLocal<UsageRegistrar> currentTracker = new ThreadLocal<>();
169176

170177
private UsageTracker() {
@@ -255,16 +262,20 @@ public static <T> T track(Supplier<T> task, UsageRegistrar tracker) {
255262
*/
256263
public static <T> @Nullable T untracked(ValueSupplier<T> task) {
257264
var previousTracker = currentTracker.get();
258-
if (previousTracker == null) {
265+
if (previousTracker == DELIBERATELY_UNTRACKED) {
259266
return task.supply();
260267
}
261268

262269
try {
263-
currentTracker.remove();
270+
currentTracker.set(DELIBERATELY_UNTRACKED);
264271

265272
return task.supply();
266273
} finally {
267-
currentTracker.set(previousTracker);
274+
if (previousTracker == null) {
275+
currentTracker.remove();
276+
} else {
277+
currentTracker.set(previousTracker);
278+
}
268279
}
269280
}
270281

@@ -282,12 +293,26 @@ public static void registerUsage(Usage usage) {
282293
}
283294

284295
/**
285-
* Checks whether a usage tracker is currently active.
296+
* Checks whether calling {@code Signal.get()} is allowed in the current
297+
* context. Returns {@code true} when a real usage tracker is active or when
298+
* inside a deliberately untracked context (e.g. via
299+
* {@code Signal.untracked()} or {@code Signal.peek()}).
300+
*
301+
* @return {@code true} if calling {@code get()} is allowed
302+
*/
303+
public static boolean isGetAllowed() {
304+
return currentTracker.get() != null;
305+
}
306+
307+
/**
308+
* Checks whether a usage tracker is currently active. Returns {@code true}
309+
* only for real trackers, not the deliberately untracked sentinel.
286310
*
287311
* @return <code>true</code> if a usage tracker is active
288312
*/
289313
public static boolean isActive() {
290-
return currentTracker.get() != null;
314+
UsageRegistrar tracker = currentTracker.get();
315+
return tracker != null && tracker != DELIBERATELY_UNTRACKED;
291316
}
292317

293318
}

flow-server/src/main/java/com/vaadin/flow/signals/local/AbstractLocalSignal.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,14 @@ protected void checkPreconditions() {
8080

8181
@Override
8282
public @Nullable T get() {
83+
if (!UsageTracker.isGetAllowed()) {
84+
throw new IllegalStateException(
85+
"Signal.get() was called outside a reactive context. "
86+
+ "Use peek() to read the value without setting up "
87+
+ "dependency tracking, or use "
88+
+ "Signal.untracked(() -> signal.get()) to "
89+
+ "explicitly opt out.");
90+
}
8391
lock.lock();
8492
try {
8593
checkPreconditions();

flow-server/src/main/java/com/vaadin/flow/signals/shared/AbstractSignal.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,14 @@ protected AbstractSignal(SignalTree tree, Id id,
156156

157157
@Override
158158
public @Nullable T get() {
159+
if (!UsageTracker.isGetAllowed() && !Transaction.inTransaction()) {
160+
throw new IllegalStateException(
161+
"Signal.get() was called outside a reactive context. "
162+
+ "Use peek() to read the value without setting up "
163+
+ "dependency tracking, or use "
164+
+ "Signal.untracked(() -> signal.get()) to "
165+
+ "explicitly opt out.");
166+
}
159167
Transaction transaction = Transaction.getCurrent();
160168
Data data = data(transaction);
161169

flow-server/src/main/java/com/vaadin/flow/signals/shared/SharedMapSignal.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -369,7 +369,7 @@ public String toString() {
369369
return "SharedMapSignal[null]";
370370
}
371371
return value.entrySet().stream()
372-
.map(entry -> entry.getKey() + "=" + entry.getValue().get())
372+
.map(entry -> entry.getKey() + "=" + entry.getValue().peek())
373373
.collect(Collectors.joining(", ", "SharedMapSignal[", "]"));
374374
}
375375

flow-server/src/main/java/com/vaadin/flow/signals/shared/SharedNumberSignal.java

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -121,14 +121,13 @@ public Double peekConfirmed() {
121121
}
122122

123123
/**
124-
* Gets the value of this signal as an integer. This method works in the
125-
* same way was {@link #get()} with regards to transactions and dependency
126-
* tracking.
124+
* Gets the value of this signal as an integer without registering a
125+
* dependency. Equivalent to {@code peek().intValue()}.
127126
*
128127
* @return the signal value as an integer
129128
*/
130129
public int getAsInt() {
131-
return get().intValue();
130+
return peek().intValue();
132131
}
133132

134133
/**

0 commit comments

Comments
 (0)