There are a couple of places where you can improve.
First the pedantic stuff.
Closable
should probably be Closeable
Cleaning up your imports can make it easier to get to the parts of your program that matter. In this case you can consolidate your imports to a much simpler version:
import scala.util.control.NonFatal
import scala.util.{Failure, Success, Try}
Make sure it compiles before posting. In this case, there was a missed rename, and it needed an additional import to compile without warnings (needed by the reflected call to #close
on a structurally typed value).
Several of your pairs of curly braces are optional, and removing them makes it much easier to read.
- Scala convention favors single character for type parameters. I'm not sure that I 100% agree with this particular convention for classes with type parameters, but for a single method it's best to stick to the convention.
Ok, now that's out of the way, to the good stuff :)
Use interfaces if they are already there
Scala's interoperability with Java is one of it's strengths, so re-inventing an interface isn't really needed. Additionally, using java.io.Closeable
gets rid of the structural typing, which is a nifty feature, but it's better to avoid if it's not strictly needed. This simplifies things considerably, particularly the type signature.
FlatMap
Try
is a Monad, which means a bunch of stuff to category theory buffs - which I don't particularly care about. What I do care about Monads is that they have two methods defined on them that can make life much easier. flatMap
is the first, and you've basically defined the content of this method in your outermost pattern match.
Refactored Version 1
import java.io.Closeable
import scala.util.control.NonFatal
import scala.util.{Failure, Success, Try}
object TryWith {
def apply [C <: Closeable, R](resGen: => C) (r: Closeable => R): Try[R] =
Try(resGen).flatMap(closeable => {
try {
Success(r(closeable))
}
catch {
case NonFatal(e) => Failure(e)
}
finally {
try {
closeable.close()
}
catch {
case e: Exception =>
System.err.println("Failed to close Resource:")
e.printStackTrace()
}
}
})
}
Principle of Least Surprise
So far we haven't changed how the method actually works. In the possibilities you mentioned for how to handle a failure to close you missed the most important option: mimic the behavior of the Java try-with-resources block.
This is important for two reasons: the first is that it's a pretty sane way of handling this case, the second is that it will be the behavior expected by anyone coming from a Java background. Acting in an intuitive manner is a very powerful thing.
Java handles failures to close in this manner:
- If the block throws an exception, give priority to this exception. The exception thrown by
#close
is added as a suppressed exception, and the modified exception is thrown.
- If the block did not throw an exception, allow the exception from close to propagate upward.
The basic premise is that exceptions are exceptional, so they should never be swallowed.
Refactored Version 2
import java.io.Closeable
import scala.util.control.NonFatal
import scala.util.{Failure, Try}
object TryWith {
def apply[C <: Closeable, R](resource: => C)(f: C => R): Try[R] =
Try(resource).flatMap(resourceInstance => {
try {
val returnValue = f(resourceInstance)
Try(resourceInstance.close()).map(_ => returnValue)
}
catch {
case NonFatal(exceptionInFunction) =>
try {
resourceInstance.close()
Failure(exceptionInFunction)
}
catch {
case NonFatal(exceptionInClose) =>
exceptionInFunction.addSuppressed(exceptionInClose)
Failure(exceptionInFunction)
}
}
})
}
The Test Suite
import java.io.Closeable
import org.scalatest.{ Matchers, WordSpec }
import scala.util.{ Failure, Success }
class TryWithSpec extends WordSpec with Matchers {
// Exceptions and errors here so we don't pay the stack trace creation cost multiple times
val getResourceException = new RuntimeException
val inFunctionException = new RuntimeException
val inCloseException = new RuntimeException
val getResourceError = new OutOfMemoryError
val inFunctionError = new OutOfMemoryError
val inCloseError = new OutOfMemoryError
val goodResource = new Closeable {
override def toString: String = "good resource"
def close(): Unit = {}
}
"TryWith" should {
"catch exceptions getting the resource" in {
TryWith(throw getResourceException)(println) shouldBe Failure(getResourceException)
}
"catch exceptions in the function" in {
TryWith(goodResource){
_ => throw inFunctionException
} shouldBe Failure(inFunctionException)
}
"catch exceptions while closing" in {
TryWith(new Closeable {
def close(): Unit = throw inCloseException
})(_.toString) shouldBe Failure(inCloseException)
}
"note suppressed exceptions" in {
val ex = new RuntimeException
val result = TryWith(new Closeable {
def close(): Unit = throw inCloseException
})(_ => throw ex)
result shouldBe Failure(ex)
val Failure(returnedException) = result
returnedException.getSuppressed shouldBe Array(inCloseException)
}
"propagate errors getting the resource" in {
intercept[OutOfMemoryError] {
TryWith(throw getResourceError)(println)
} shouldBe getResourceError
}
"propagate errors in the function" in {
intercept[OutOfMemoryError] {
TryWith(goodResource){
_ => throw inFunctionError
}
} shouldBe inFunctionError
}
"propagate errors while closing" in {
intercept[OutOfMemoryError] {
TryWith(new Closeable {
def close(): Unit = throw inCloseError
})(_.toString)
} shouldBe inCloseError
}
"return the value from a successful run" in {
TryWith(goodResource)(_.toString) shouldBe Success("good resource")
}
}
}
resGen: => Closable
rather thanresGen: => T
, correct? \$\endgroup\$