RFR: 8312436: CompletableFuture never completes when 'Throwable.toString()' method throws Exception

[Viktor Klang]

On Sun, 28 Apr 2024 09:54:34 GMT, Viktor Klang <vklang at openjdk.org> wrote:

> Primarily offering this PR for discussion, as Throwables throwing exceptions on toString(), getLocalizedMessage(), or getMessage() seems like a rather unreasonable thing to do.
> 
> Nevertheless, there is some things we can do, as witnessed in this PR.

src/java.base/share/classes/java/util/concurrent/CompletableFuture.java line 352:

> 350:         return wrapping;
> 351:     }
> 352: 

This is the main thing—and the big question is if this approach is the best path or not.

src/java.base/share/classes/java/util/concurrent/CompletableFuture.java line 2653:

> 2651:      * its completion state.  The state, in brackets, contains the
> 2652:      * String {@code "Completed normally"} or the String {@code
> 2653:      * "Completed exceptionally"}, or the String {@code "Not

These two looks like a discrepancy between spec and implementation, and it seemed safer to amend the spec rather than the implementation, as code might be relying on *actual behavior* rather than the spec:ed one.

@DougLea You might have some thoughts to offer on this :)

src/java.base/share/classes/java/util/concurrent/CompletableFuture.java line 2670:

> 2668:                 : "[Not completed, " + count + " dependents]")
> 2669:              : (((r instanceof AltResult) && ((AltResult)r).ex != null)
> 2670:                 ? "[Completed exceptionally: " + ((AltResult)r).ex.getClass().getName() + "]"

@DougLea @AlanBateman It seemed safer to switch to outputting the cause FQCN rather than wrapping in a try-catch and trying to come up with a fallback text.

The reason for amending this is for things like JShell, results are toString():ed which would mean that the current session would break if toString():ing the result throws (also further signalling that throwing on toString() seems unreasonable in general).

test/jdk/java/util/concurrent/tck/CompletableFutureTest.java line 87:

> 85:         // even if thrown exceptions end up throwing exceptions from their String
> 86:         // representations.
> 87:         @Override public String getMessage() {

I decided to do this to make sure that *all pre-existing test cases* would still work even if the exception used has a misbehaving toString() (toString() transitively calls getLocalizedMessage() which in turn calls getMessage())

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/18988#discussion_r1582075756
PR Review Comment: https://git.openjdk.org/jdk/pull/18988#discussion_r1582075209
PR Review Comment: https://git.openjdk.org/jdk/pull/18988#discussion_r1582075561
PR Review Comment: https://git.openjdk.org/jdk/pull/18988#discussion_r1582075630