Unfinalize the class DefaultPromise #4690
Conversation
It was non-final in Scala 2.11.x, and made final as part of fa0743c. Removing the final modifier seems like the cleanest way to enable conversions like `javaFuture.toScala.toJava` to return the original `javaFuture` in scala-java8-compat. I have made the methods defined in this class final as an alternative lockdown. Discussion, Motivation: scala/scala-java8-compat#46 scala/scala-java8-compat#50
retronym
force-pushed
the
topic/defaultpromise
branch
from
87f95ec to
898ee2a
Compare
| final class DefaultPromise[T] extends AtomicReference[AnyRef](Nil) with Promise[T] { | ||
| // Left non-final to enable addition of extra fields by Java/Scala converters | ||
| // in scala-java8-compat. | ||
| class DefaultPromise[T] extends AtomicReference[AnyRef](Nil) with Promise[T] { |
There was a problem hiding this comment.
One thing to keep in mind is that this class extends AtomicReference (for performance reasons, since VarHandles are not available and AtomicReferenceUpdaters are slow) so subclasses could technically override the methods on AtomicReference (or worse, mess around with them).
I think we ought to put a big WARNING sign that says "don't mess around with the innards"
There was a problem hiding this comment.
@retronym Could you verify that the transitively inherited methods from Promise are final too?
There was a problem hiding this comment.
IMO it is enough that this is private[concurrent]
There was a problem hiding this comment.
I'm more worried about Java users here.
There was a problem hiding this comment.
but AtomicReferenceUpdater will save memory.
There was a problem hiding this comment.
Promise/Future helps a lot,change the way of asynchronous programming with scala,thanks @viktorklang .opt of Promise should be a part of slip,I think.
There was a problem hiding this comment.
I'm more worried about Java users here.
We don't compile qualified private into JVM private.
$ 2.12.0-M2 javap scala/concurrent/impl/Promise\$DefaultPromise.class
Compiled from "Promise.scala"
public final class scala.concurrent.impl.Promise$DefaultPromise<T> extends java.util.concurrent.atomic.AtomicReference<java.lang.Object> implements scala.concurrent.impl.Promise<T> {
There was a problem hiding this comment.
I'm more worried about Java users here.
Are you suggesting that I add final overriding methods that delegate to the super method for each of (or some of) these:
That seems quite heavyweight to me. What are we seeking to prevent?
There was a problem hiding this comment.
I'm probably overthinking this. I think we'll be fine.
|
@viktorklang Are you happy to give this one a LGTM? |
Makes `toJava.toScala` and `toScala.toJava` a wrap-and-unwrap operation, rather than a wrap-and-rewrap. Once 2.12.0-M3 is released, we can reduce the overhead by extending `DefaultPromise` rather than `Promise`. See: 4faeac5 scala/scala#4690
Makes `toJava.toScala` and `toScala.toJava` a wrap-and-unwrap operation, rather than a wrap-and-rewrap. Once 2.12.0-M3 is released, we can reduce the overhead by extending `DefaultPromise` rather than `Promise`. See: 4faeac5 scala/scala#4690
|
@retronym Apologies, of course, LGTM! |
|
LGTM echo, for the purposes of the bot. |
Unfinalize the class DefaultPromise
Makes `toJava.toScala` and `toScala.toJava` a wrap-and-unwrap operation, rather than a wrap-and-rewrap. Removes the cross build on 2.12, we should wait until 2.12.0-M3 in which DefaultPromise is once again extendable. See: 4faeac5 scala/scala#4690
Makes `toJava.toScala` and `toScala.toJava` a wrap-and-unwrap operation, rather than a wrap-and-rewrap. Removes the cross build on 2.12, we should wait until 2.12.0-M3 in which DefaultPromise is once again extendable. See: 4faeac5 scala/scala#4690
Makes `toJava.toScala` and `toScala.toJava` a wrap-and-unwrap operation, rather than a wrap-and-rewrap. Removes the cross build on 2.12, we should wait until 2.12.0-M3 in which DefaultPromise is once again extendable. See: retronym@4faeac5 scala/scala#4690
It was non-final in Scala 2.11.x, and made final as part
of fa0743c.
Removing the final modifier seems like the cleanest way to enable
conversions like
javaFuture.toScala.toJavato return the originaljavaFuturein scala-java8-compat.I have made the methods defined in this class final as an
alternative lockdown.
Discussion, Motivation:
scala/scala-java8-compat#46
scala/scala-java8-compat#50
Review by @viktorklang