Merge pull request #175 from argv-minus-one/managed-resource-exceptio…

…n-handling

Managed resource exception handling - fix for #173

core/src/main/scala/better/files/ManagedResource.scala

@@ -3,6 +3,7 @@ package better.files
 import java.util.concurrent.atomic.AtomicBoolean
 
 import scala.util.Try
+import scala.util.control.NonFatal
 
 /**
   * A typeclass to denote a disposable resource
@@ -32,16 +33,39 @@ object Disposable {
 
 class ManagedResource[A](resource: A)(implicit disposer: Disposable[A]) {
   private[this] val isDisposed = new AtomicBoolean(false)
-  private[this] def disposeOnce() = if (!isDisposed.getAndSet(true)) disposer.disposeSilently(resource)
+  private[this] def disposeOnce() = if (!isDisposed.getAndSet(true)) disposer.dispose(resource)
 
   def foreach[U](f: A => U): Unit = {
     val _ = map(f)
   }
 
   def map[B](f: A => B): B = {
-    val result = Try(f(resource))
-    disposeOnce()
-    result.get
+    // Avoid using Option here. If an OutOfMemoryError is caught, allocating an Option may cause another one.
+    var e1: Throwable = null
+
+    // This is the Scala equivalent of how javac compiles try-with-resources, except that fatal exceptions while disposing take precedence over exceptions thrown by the provided function.
+    try
+      f(resource)
+    catch { case e: Throwable =>
+      e1 = e
+      throw e
+    }
+    finally {
+      if (e1 ne null) {
+        try
+          disposeOnce()
+        catch { case e2: Throwable =>
+          if (NonFatal(e2))
+            e1.addSuppressed(e2)
+          else {
+            e2.addSuppressed(e1)
+            throw e2
+          }
+        }
+      }
+      else
+        disposeOnce()
+    }
   }
 
   def withFilter(f: A => Boolean): this.type = {
@@ -60,9 +84,25 @@ class ManagedResource[A](resource: A)(implicit disposer: Disposable[A]) {
   def flatMap[B](f: A => Iterator[B]): Iterator[B] = {
     val it = f(resource)
     it withHasNext {
-      val result = Try(it.hasNext)
-      if (!result.getOrElse(false)) disposeOnce()
-      result.get
+      try {
+        val result = it.hasNext
+        if (!result) disposeOnce()
+        result
+      }
+      catch { case e1: Throwable =>
+        try
+          disposeOnce()
+        catch { case e2: Throwable =>
+          if (NonFatal(e2))
+            e1.addSuppressed(e2)
+          else {
+            e2.addSuppressed(e1)
+            throw e2
+          }
+        }
+
+        throw e1
+      }
     }
   }
 }

core/src/test/scala/better/files/ManagedResourceSpec.scala

@@ -0,0 +1,250 @@
+package better.files
+
+import org.scalatest.matchers.{MatchResult, Matcher}
+
+import scala.reflect.ClassTag
+import scala.util.control.ControlThrowable
+
+class ManagedResourceSpec extends CommonSpec {
+  // Test classes
+
+  private class TestDisposable extends AutoCloseable {
+    var closeCount = 0
+
+    override def close(): Unit =
+      closeCount += 1
+  }
+
+  private class TestDisposableThatThrows extends TestDisposable {
+    override def close(): Unit = {
+      super.close()
+      throw new TestDisposeException
+    }
+  }
+
+  private class TestDisposableThatThrowsFatal extends TestDisposable {
+    override def close(): Unit = {
+      super.close()
+      throw new TestDisposeFatalException
+    }
+  }
+
+  private class TestEvalException extends Exception
+  private class TestDisposeException extends Exception
+  private class TestDisposeFatalException extends Exception with ControlThrowable
+
+  // Custom matchers
+
+  private class HaveSuppressedMatcher(classes: Class[_ <: Throwable]*) extends Matcher[Throwable] {
+    override def apply(left: Throwable): MatchResult = {
+      MatchResult(
+        (classes corresponds left.getSuppressed) {
+          (clazz, suppressed) => clazz isInstance suppressed
+        },
+        s"had suppressed exceptions of types ${classes.map(_.getSimpleName).mkString(", ")}",
+        s"had not suppressed exceptions of types ${classes.map(_.getSimpleName).mkString(", ")}"
+      )
+    }
+  }
+
+  private def haveSuppressed[E <: Throwable](implicit ct: ClassTag[E]) =
+    new HaveSuppressedMatcher(ct.runtimeClass.asInstanceOf[Class[_ <: Throwable]])
+
+  // Test body
+
+  behavior of "managed resources"
+
+  it should "map correctly" in {
+    val t = new TestDisposable
+
+    val result = for {
+      tc <- t.autoClosed
+    } yield {
+      t.closeCount shouldBe 0
+      "hello"
+    }
+
+    result shouldBe "hello"
+    t.closeCount shouldBe 1
+  }
+
+  it should "flatMap correctly" in {
+    val t = new TestDisposable
+
+    val result = (for {
+      tc <- t.autoClosed
+      v <- Iterator("one", "two", "three")
+    } yield {
+      t.closeCount shouldBe 0
+      v
+    }).toSeq
+
+    result should contain inOrder ("one", "two", "three")
+    t.closeCount shouldBe 1
+  }
+
+  it should "handle exceptions correctly" in {
+    val t = new TestDisposable
+
+    a [TestEvalException] should be thrownBy {
+      for {
+        tc <- t.autoClosed
+      } {
+        t.closeCount shouldBe 0
+        throw new TestEvalException
+      }
+    }
+    t.closeCount shouldBe 1
+
+    var lastSeen = ""
+    a [TestEvalException] should be thrownBy {
+      for {
+        tc <- t.autoClosed
+        v <- Iterator("one", "two", "three")
+      } {
+        t.closeCount shouldBe 1
+        lastSeen = v
+        if (v == "two") throw new TestEvalException
+      }
+    }
+    t.closeCount shouldBe 2
+    lastSeen shouldBe "two"
+  }
+
+  it should "handle disposal exceptions correctly" in {
+    // For some mysterious reason, thrownBy doesn't work here, in this specific test case. No clue why, despite spending an entire day trying to figure it out,
+    // including repeatedly stepping through the innards of ScalaTest in a debugger. Catching the exception manually does work, though.
+    val messageNoException = "no exception was thrown"
+    def messageWrongException(e: Throwable): String =
+      s"an exception was thrown, but not a TestDisposeException; instead it's a ${e.getClass.getName}"
+
+    val t = new TestDisposableThatThrows
+
+    val e1 =
+      try {
+        for {
+          tc <- t.autoClosed
+        } {
+          t.closeCount shouldBe 0
+        }
+        None
+      }
+      catch {
+        case e: TestDisposeException =>
+          Some(e)
+      }
+    assert(e1.nonEmpty, messageNoException)
+    e1 foreach { e1c => assert(e1c.isInstanceOf[TestDisposeException], messageWrongException(e1c)) }
+    t.closeCount shouldBe 1
+
+    var lastSeen = ""
+    val e2 =
+      try {
+        val i = for {
+          tc <- t.autoClosed
+          v <- Iterator("one", "two", "three")
+        } yield {
+          t.closeCount shouldBe 1
+          lastSeen = v
+          v
+        }
+        while (i.hasNext) i.next()
+        None
+      }
+      catch {
+        case e: TestDisposeException =>
+          Some(e)
+      }
+    lastSeen shouldBe "three"
+    assert(e2.nonEmpty, messageNoException)
+    e2 foreach { e2c => assert(e2c.isInstanceOf[TestDisposeException], messageWrongException(e2c)) }
+    t.closeCount shouldBe 2
+  }
+
+  it should "handle non-local returns correctly" in {
+    val t = new TestDisposable
+
+    def doTheThing(): String = {
+      throw the [ControlThrowable] thrownBy {
+        for {
+          tc <- t.autoClosed
+        } {
+          t.closeCount shouldBe 0
+          return "hello"
+        }
+      }
+    }
+    doTheThing() shouldBe "hello"
+    t.closeCount shouldBe 1
+
+    def doTheThings(): String = {
+      throw the [ControlThrowable] thrownBy {
+        for {
+          tc <- t.autoClosed
+          v <- Iterator("one", "two", "three")
+        } {
+          t.closeCount shouldBe 1
+          if (v == "two") return v
+        }
+      }
+    }
+    doTheThings() shouldBe "two"
+    t.closeCount shouldBe 2
+  }
+
+  it should "handle multiple exceptions correctly" in {
+    val t = new TestDisposableThatThrows
+
+    the [TestEvalException] thrownBy {
+      for {
+        tc <- t.autoClosed
+      } {
+        t.closeCount shouldBe 0
+        throw new TestEvalException
+      }
+    } should haveSuppressed [TestDisposeException]
+    t.closeCount shouldBe 1
+
+    var lastSeen = ""
+    the [TestEvalException] thrownBy {
+      for {
+        tc <- t.autoClosed
+        v <- Iterator("one", "two", "three")
+      } {
+        t.closeCount shouldBe 1
+        lastSeen = v
+        if (v == "two") throw new TestEvalException
+      }
+    } should haveSuppressed [TestDisposeException]
+    lastSeen shouldBe "two"
+    t.closeCount shouldBe 2
+  }
+
+  it should "give fatal exceptions precedence" in {
+    val t = new TestDisposableThatThrowsFatal
+
+    the [TestDisposeFatalException] thrownBy {
+      for {
+        tc <- t.autoClosed
+      } {
+        t.closeCount shouldBe 0
+        throw new TestEvalException
+      }
+    } should haveSuppressed [TestEvalException]
+    t.closeCount shouldBe 1
+
+    var lastSeen = ""
+    the [TestDisposeFatalException] thrownBy {
+      for {
+        tc <- t.autoClosed
+        v <- Iterator("one", "two", "three")
+      } {
+        t.closeCount shouldBe 1
+        lastSeen = v
+        if (v == "two") throw new TestEvalException
+      }
+    } should haveSuppressed [TestEvalException]
+    t.closeCount shouldBe 2
+    lastSeen shouldBe "two"
+  }
+}