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.

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

Due to Duration.Inf being a specific instance, Promise.tryAwait(...) fails with a MatchError if Duration.Inf is serialized and deserialized (e.g. using Akka remotely) #9197

Closed
scabug opened this issue Mar 4, 2015 · 6 comments
Assignees
Milestone

Comments

@scabug
Copy link

scabug commented Mar 4, 2015

scala.concurrent.duration.Duration.Inf is a specific instance - i.e.

val Inf: Infinite = new Infinite {
    override def toString = "Duration.Inf"
    def compare(other: Duration) = other match {
      case x if x eq Undefined => -1 // Undefined != Undefined
      case x if x eq this      => 0  // `case Inf` will include null checks in the byte code
      case _                   => 1
    }
    def unary_- : Duration = MinusInf
    def toUnit(unit: TimeUnit): Double = Double.PositiveInfinity
  }

This all works brilliantly unless you send it from one VM to another (say via a remote Akka call). If you do, it creates a new "Duration.Inf" instance, so now it is no longer a singleton in the receiving VM.

If you then pass this new Duration.Inf into Await.result or something you receive a MatchError in scala.concurrent.impl.Promise.tryAwait(...) here:

    /** Try waiting for this promise to be completed.
     */
    protected final def tryAwait(atMost: Duration): Boolean = if (!isCompleted) {
      import Duration.Undefined
      import scala.concurrent.Future.InternalCallbackExecutor
      atMost match {
        case e if e eq Undefined => throw new IllegalArgumentException("cannot wait for Undefined period")
        case Duration.Inf        =>
          val l = new CompletionLatch[T]()
          onComplete(l)(InternalCallbackExecutor)
          l.acquireSharedInterruptibly(1)
        case Duration.MinusInf   => // Drop out
        case f: FiniteDuration   =>
          if (f > Duration.Zero) {
            val l = new CompletionLatch[T]()
            onComplete(l)(InternalCallbackExecutor)
            l.tryAcquireSharedNanos(1, f.toNanos)
          }
      }

      isCompleted
    } else true // Already completed

i.e. It attempts to match the Duration.Inf instances and fails because they are different - and you end up with a confusing error saying "MatchError: Duration.Inf" even though there is clearly a match case for it.

@scabug
Copy link
Author

scabug commented Mar 4, 2015

Imported From: https://issues.scala-lang.org/browse/SI-9197?orig=1
Reporter: Kim Goodman (kalgoodman)
Affected Versions: 2.11.4

@scabug
Copy link
Author

scabug commented Mar 31, 2015

@Ichoran said:
I think overriding readResolve will solve this one. Not technically binary compatible, I don't think, but it's going to be awfully tricky to do it otherwise. Only string identity is safe, and users might also employ the eq Inf pattern.

@scabug
Copy link
Author

scabug commented Mar 31, 2015

@Ichoran said:
Minimization illustrating the problem:

val baos = new java.io.ByteArrayOutputStream
val out = new java.io.ObjectOutputStream(baos)
out.writeObject(scala.concurrent.duration.Duration.Inf)
val bais = new java.io.ByteArrayInputStream( baos.toByteArray )
val in = new java.io.ObjectInputStream(bais)
in.readObject eq scala.concurrent.duration.Duration.Inf

This returns false, but should return true.

@scabug
Copy link
Author

scabug commented Mar 31, 2015

@Ichoran said:
Well, the fix fixes the problem, but it's outrageously incompatible because the autogenerated serialVersionUID changes when you add readResolve. So--boom!--you can't pass these around. Finding and fixing every "eq Inf" is messy and painful.

It's possible, I guess, to ask ASM to rewrite the bytecode of scala.concurrent.duration.Duration$$anon$1 through 3 to contain a private static final long serialVersionUID of the right value. But, ugh?

@scabug
Copy link
Author

scabug commented Mar 31, 2015

@Ichoran said:
scala/scala#4416

@scabug
Copy link
Author

scabug commented Mar 31, 2015

@Ichoran said:
Turns out readResolve can be private, so even though the consistency of serialVersionUID is still in question, it at least isn't changed by adding the readResolves that turn the serialized copy into just a reference to the local singleton.

@scabug scabug closed this as completed Apr 23, 2015
@scabug scabug added this to the 2.11.7 milestone Apr 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants