Re: RFR: 8312436: CompletableFuture never completes when 'Throwable.toString()' method throws Exception
On Sun, 28 Apr 2024 09:54:34 GMT, Viktor Klang 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. Will this be backported to JDK21U? - PR Comment: https://git.openjdk.org/jdk/pull/18988#issuecomment-2196143083
Re: RFR: 8312436: CompletableFuture never completes when 'Throwable.toString()' method throws Exception
On Sun, 28 Apr 2024 09:54:34 GMT, Viktor Klang 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. Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18988#pullrequestreview-2099417935
Re: RFR: 8312436: CompletableFuture never completes when 'Throwable.toString()' method throws Exception
On Sun, 28 Apr 2024 09:54:34 GMT, Viktor Klang 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. @AlanBateman Do you have a minute to review this one? - PR Comment: https://git.openjdk.org/jdk/pull/18988#issuecomment-2149910528
Re: RFR: 8312436: CompletableFuture never completes when 'Throwable.toString()' method throws Exception
On Mon, 29 Apr 2024 11:48:08 GMT, Chen Liang 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
> 310:
>
>> 308:
>> 309: static CompletionException wrapInCompletionException(Throwable t) {
>> 310: if (t == null)
>
> Is there any preexisting code path that ever passes a null? If not I don't
> think this check is necessary.
I opted to be more safe than sorry. Since this is on the failure-path it isn't
performance critical so I think affording a null-check is fine.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18988#discussion_r1583299298
Re: RFR: 8312436: CompletableFuture never completes when 'Throwable.toString()' method throws Exception
On Sun, 28 Apr 2024 09:54:34 GMT, Viktor Klang 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
Re: RFR: 8312436: CompletableFuture never completes when 'Throwable.toString()' method throws Exception
On Sun, 28 Apr 2024 09:54:34 GMT, Viktor Klang 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
310:
> 308:
> 309: static CompletionException wrapInCompletionException(Throwable t) {
> 310: if (t == null)
Is there any preexisting code path that ever passes a null? If not I don't
think this check is necessary.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18988#discussion_r1582947192
RFR: 8312436: CompletableFuture never completes when 'Throwable.toString()' method throws Exception
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. - Commit messages: - CompletableFuture should be more resilient against misbehaving exceptions Changes: https://git.openjdk.org/jdk/pull/18988/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18988=00 Issue: https://bugs.openjdk.org/browse/JDK-8312436 Stats: 63 lines in 2 files changed: 52 ins; 2 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/18988.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18988/head:pull/18988 PR: https://git.openjdk.org/jdk/pull/18988
