Skip to content
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.

Sign up for GitHub

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

Jump to bottom

=lib Make use of CompletionStage#handle instead of whenComplete #10181

Merged
merged 1 commit into from
Nov 25, 2022
Merged

=lib Make use of CompletionStage#handle instead of whenComplete #10181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions project/MimaFilters.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,13 @@ object MimaFilters extends AutoPlugin {
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.Predef#ArrayCharSequence.isEmpty"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.runtime.ArrayCharSequence.isEmpty"),

// KEEP: make use of CompletionStage#handle to get a better performance than CompletionStage#whenComplete.
ProblemFilters.exclude[MissingTypesProblem]("scala.concurrent.impl.FutureConvertersImpl$P"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.concurrent.impl.FutureConvertersImpl#P.andThen"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.concurrent.impl.FutureConvertersImpl#P.apply"),
ProblemFilters.exclude[IncompatibleMethTypeProblem]("scala.concurrent.impl.FutureConvertersImpl#P.andThen"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.concurrent.impl.FutureConvertersImpl#P.accept"),
ProblemFilters.exclude[IncompatibleMethTypeProblem]("scala.concurrent.impl.FutureConvertersImpl#P.andThen"),
)

override val buildSettings = Seq(
Expand Down
4 changes: 2 additions & 2 deletions src/library/scala/concurrent/impl/FutureConvertersImpl.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,8 @@ private[scala] object FutureConvertersImpl {
override def toString(): String = super[CompletableFuture].toString
}

final class P[T](val wrapped: CompletionStage[T]) extends DefaultPromise[T] with BiConsumer[T, Throwable] {
override def accept(v: T, e: Throwable): Unit = {
final class P[T](val wrapped: CompletionStage[T]) extends DefaultPromise[T] with BiFunction[T, Throwable, Unit] {
override def apply(v: T, e: Throwable): Unit = {
if (e == null) success(v)
else failure(e)
}
Expand Down
10 changes: 7 additions & 3 deletions src/library/scala/jdk/javaapi/FutureConverters.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@

package scala.jdk.javaapi

import java.util.concurrent.CompletionStage

import java.util.concurrent.{CompletableFuture, CompletionStage}
import scala.concurrent.impl.FutureConvertersImpl.{CF, P}
import scala.concurrent.{ExecutionContext, Future}
import scala.util.Success

/** This object contains methods that convert between Scala [[scala.concurrent.Future]] and Java [[java.util.concurrent.CompletionStage]].
*
Expand Down Expand Up @@ -70,9 +70,13 @@ object FutureConverters {
case cf: CF[T] => cf.wrapped
// in theory not safe (could be `class C extends Future[A] with CompletionStage[B]`):
case f: Future[T @unchecked] => f
case cf: CompletableFuture[T @unchecked] if cf.isDone && !cf.isCompletedExceptionally =>
Copy link

Choose a reason for hiding this comment

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

FYI @He-Pin
JDK 11 has MinimalStage which is a CompletableFuture (also CompletionStage), with UnsupportedOperationException in lot of method bodies (like isDone).
With this change we throw from here at line 73 if encounered a MinimalStage.

I am not claiming this is a bug, or MinimalStage is flawed; neither know whether it is a valid case to have MinimalStage flying around, just letting you know.
Don't shoot the messenger please ;)
Others also implement safety measures: Kotlin/kotlinx.coroutines#2456

Copy link
Member

Choose a reason for hiding this comment

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

Many thanks @nmarton-da. It reproduces indeed:

scala> import scala.jdk.FutureConverters._
import scala.jdk.FutureConverters._

scala> import java.util.concurrent.CompletableFuture
import java.util.concurrent.CompletableFuture

scala> CompletableFuture.completedStage(42).asScala
java.lang.UnsupportedOperationException
  at java.base/java.util.concurrent.CompletableFuture$MinimalStage.isDone(CompletableFuture.java:2976)
  at scala.jdk.javaapi.FutureConverters$.asScala(FutureConverters.scala:73)
  at scala.jdk.FutureConverters$CompletionStageOps$.asScala$extension(FutureConverters.scala:41)
  ... 30 elided

Copy link
Member

Choose a reason for hiding this comment

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

It seems play framework had to work around it: playframework/playframework#11993

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay, I was busy at shopping festival, will submit fix to other repository this weekend.

val p = new P[T](cs)
p.tryComplete(Success(cf.join()))
p.future
case _ =>
val p = new P[T](cs)
cs whenComplete p
cs.handle(p)
p.future
}
}
Expand Down