Stop a failed future rendering as "Completed", and a tuple of none rendering as NPE - #649
Merged
Merged
Conversation
lagergren
force-pushed
the
lagergren/569-8-future
branch
from
September 26, 2026 09:07
c91a2b8 to
f59f00c
Compare
ggleyzer
approved these changes
Sep 26, 2026
ggleyzer
reviewed
Sep 26, 2026
…ndering as NPE
FutureHandle.toString() described a done future by calling toSafeString():
return String.valueOf(getFuture().get());
// catch (Throwable e) -> Utils.translate(e).toString()
Three problems, only one of which is about purity.
A future that has failed is done, so this took the catch branch and produced
"Completed: <translated exception>" - the word "Completed" about a failure. A cancelled future is
also done, and rendered the same way.
FutureTupleHandle inherits this toString(), and its getFuture() returns null when f_ahValue holds no
FutureHandle, so toString() threw NullPointerException on the isDone() call. Rendering a value is
not a reasonable place to throw.
Utils.translate(e) constructs an exception handle in the owning container, from whatever thread
happened to be rendering. That is the impurity, and it only ever ran to build a string that was
mislabelled anyway.
describe() answers from the future's own state - absent, pending, cancelled, failed, or completed -
using getNow(null), which neither blocks nor throws. Cancellation is tested before exceptional
completion because a cancelled future is also completed exceptionally. Behaviour is unchanged for
the two cases that were already right: a pending future still reads "Not completed", and a
successfully completed one still reads "Completed: <value>".
toSafeString() had no other callers and is removed with it.
The test exercises describe() directly rather than through a handle, since the point of the method
is that it needs no container, composition or running service. Four of its six cases fail against
the previous implementation - the absent, cancelled and failed ones - and the two that pass are the
two whose behaviour must not change, which is what they are there to pin.
lagergren
force-pushed
the
lagergren/569-8-future
branch
from
September 26, 2026 21:34
6ec1ef4 to
8780a08
Compare
ggleyzer
approved these changes
Sep 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Future display currently throws for a
FutureTupleHandlecontaining no futures and labels failed or cancelled futures as "Completed". Rendering those failures also callsUtils.translate()to construct an exception handle solely for its display text.This change checks the future's state before reading a successful result. The final diff changes only
xFuture.java: +23 / −8. It continues the display series (#640–#643, #648).<no future>Not completedCompleted: <value>Completed: <translated exception><cancelled>Completed: <translated exception><failed>Cancellation is checked before exceptional completion, since cancellation is also exceptional completion. Successful results use
getNow(null); the unusedtoSafeString()is removed. The previousget()call was guarded byisDone(), so this fixes display behavior, not a blocking read.Verification
After removing the standalone test fixture:
Before its removal, the standalone fixture exercised real handles in a native container. With
xFuture.javareplaced byte-for-byte by the then-current master implementation, 2 of 4 tests failed: the tuple display threw an NPE, and failed-future display accessed the stored exception's message. Restoring the fix made all four pass with zero skips. This is historical reproduction evidence; the fixture is no longer included in the final diff.Suggested follow-up coverage
The 134-line standalone fixture was removed following review to keep this small fix focused. This PR therefore leaves no dedicated automated regression test for future display.
A shared Java runtime/debugger-display smoke suite could initialize a native container once and exercise actual handle
toString()methods across handle types. Future cases should include tuples with no futures and pending, successful, failed and cancelled futures. Assertions should check that display succeeds, preserves state and avoids rendering stored failures, without prescribing exact labels or punctuation.Ordinary
.xfuture tests do not exercise Java handletoString(), so they cannot replace that coverage. The shared suite is a proposal, not part of this PR.