Skip to content

Avoid completableFuture.asScala crash for MinimalStage #10615

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 20, 2023

Conversation

lrytz
Copy link
Member

@lrytz lrytz commented Nov 30, 2023

CompletableFuture.completedStage creates a MinimalStage instance which extends CompletableFuture and overrides most methods with UnsupportedOperationException.

Calling toCompletableFuture on it creates a fresh CompletableFuture.

Fixes scala/bug#12918.

@scala-jenkins scala-jenkins added this to the 2.13.14 milestone Nov 30, 2023
@lrytz lrytz modified the milestones: 2.13.14, 2.13.13 Nov 30, 2023
@lrytz lrytz requested a review from He-Pin November 30, 2023 11:19
@SethTisue SethTisue added the library:concurrent Changes to the concurrency support in stdlib label Nov 30, 2023
@He-Pin
Copy link
Contributor

He-Pin commented Nov 30, 2023

I will fix it in other repo this weekend

@He-Pin
Copy link
Contributor

He-Pin commented Dec 1, 2023

I was missed this @DougLea

Copy link
Contributor

@He-Pin He-Pin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@som-snytt
Copy link
Contributor

Thanks for the opportunity to read up on these interfaces I'd managed to avoid all these years.

The issue is exactly this concluding line of JavaDoc:

This interface does not define methods for initially creating, forcibly completing normally or exceptionally, probing completion status or results, or awaiting completion of a stage. Implementations of CompletionStage may provide means of achieving such effects, as appropriate. Method toCompletableFuture enables interoperability among different implementations of this interface by providing a common conversion type.

That is obscure, but says don't call isDone. If you want to do these things, use toCompletableFuture. The phrase "common conversion type" is not meaningful to me, even though the context is conversions. But presumably it means what you already discovered, that there may be various CompletionStage implementations of variable abilities, and convert to a CF for interoperability.

The other "object lesson" (pun alert) is why did we ever think it's OK to downcast to runtime types? On the one hand, we can despise Java's penchant for implementing an interface by throwing unsupported operations, such as iterator.remove when it's not supported by a collection. On the other hand, we can only trust the (compile time) type we're given as a value parameter. The exception is probing for the private wrapper type.

Copy link
Contributor

@som-snytt som-snytt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Despite the hiccup, it was pleasant to see the previous optimization and the benchmarking.

My last question is why do we downcast at all if I can always call CompletionStage#toCompetableFuture#isDone? Any completion stage can tell me (via the "common conversion type") whether a value is available.

Since it's only an optimization of sorts, approving forthwith.

@lrytz
Copy link
Member Author

lrytz commented Dec 20, 2023

My last question is why do we downcast at all if I can always call CompletionStage#toCompetableFuture#isDone? Any completion stage can tell me (via the "common conversion type") whether a value is available.

Maybe toCompetableFuture is more likely to be a no-op if we only call it on CompletableFutures, so there could be a performance benefit? I don't know.

@lrytz lrytz merged commit 85fa13d into scala:2.13.x Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
library:concurrent Changes to the concurrency support in stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FutureConverters fails for MinimalStage
5 participants