9

I have the following dummy Scala code in the file test.scala:

class Transaction {
  def begin() {}
  def commit() {}
  def rollback() {}
}

object Test extends Application {
  def doSomething() {}

  val t = new Transaction()
  t.begin()
  try {
    doSomething()
    t.commit()
  } catch {
    case _ => t.rollback()
  }
}

If I compile this on Scala 2.8 RC1 with scalac -Xstrict-warnings test.scala I'll get the following warning:

test.scala:16: warning: catch clause swallows everything: not advised.
    case _ => t.rollback()
    ^
one warning found

So, if catch-all expressions are not advised, how am I supposed to implement such a pattern instead? And apart from that why are such expressions not advised anyhow?

Michel Krämer
  • 14,197
  • 5
  • 32
  • 32

4 Answers4

9

The warning exists because you probably don't want to catch everything. For example, it's generally inadvisable to try to catch anything in java.lang.Error since it's often hard to recover from such things. (Chances are good that you'll be thrown out of your catch block with another exception.)

Also, because you can't usefully catch everything, this isn't a safe way to implement atomic/failsafe transactions. You're better off with something like

try {
  t.commit()
} finally {
  if (!t.checkCommitted()) {
    t.rollback()
    if (!t.checkRolledback()) throw new FUBARed(t)
  }
}

with additional testing when reading in a new t to make sure it's in a sensible state.

Rex Kerr
  • 166,841
  • 26
  • 322
  • 407
  • OK. This one works for transactions. But what if I want to completely ignore an exception thrown by a method just because it doesn't matter at this point. – Michel Krämer Apr 28 '10 at 12:20
  • 5
    You can `catch { case _: Exception => }`. `Error` is `Throwable` but not an `Exception`--usually better to go through. If you really mean, "I don't care if I attempt and fail to catch this, I at least want to give it my best shot", then you can live with the (strict) warning message. That's why it's a warning, not an error. – Rex Kerr Apr 28 '10 at 14:20
  • Yes, that works. Thanks! You're right, that catching `Error` is not really what I want :-) – Michel Krämer Apr 28 '10 at 17:06
  • 1
    For catch-all handlers, you should probably prefer using `NonFatal` (see http://www.scala-lang.org/api/current/index.html#scala.util.control.NonFatal$). This will integrate better with scala. In particular, this won't interfer with returns from within a closure, which under the hood throws a `NonLocalReturnControl` – Régis Jean-Gilles Mar 20 '14 at 15:52
  • @RégisJean-Gilles - Indeed, that's the way to do it now. – Rex Kerr Mar 20 '14 at 16:26
2

I don't have a compiler to hand to test this, but shouldn't you be re-throwing the exception after rolling back the transaction? i.e. this should be

val t = new Transaction()
t.begin()
try {
  doSomething()
  t.commit()
} catch {
  case e => t.rollback(); throw e
}

If you are catching all exceptions, you should take note of the documentation for ControlThrowable. Presumably you want your transaction to roll back on abnormal termination, but wouldn't want it to roll back for a non-local return or a util.control.Breaks.break. If so, you might want to do something like the following:

val t = new Transaction()
t.begin()
try {
  doSomething()
  t.commit()
} catch {
  case ce : ControlThrowable => throw ce // propagate
  case e => t.rollback(); throw e        // roll-back and propagate
}
Ben Lings
  • 28,823
  • 13
  • 72
  • 81
1

You have to catch Throwable to state your intent of catching all:

  try {
     android.util.Log.i (TAG, "Feature " + Text)
     statements
  }
  catch {
     case exception: Throwable =>
        val Message = "Feature " + Text + "failed"
        android.util.Log.e (TAG, Message, exception)
        fail (Message)
  } // try

The example above if from a piece of unit test. As the warning says: not advised in normal code

Martin
  • 11,577
  • 16
  • 80
  • 110
1

First, notice that this is a warning, not an error. And even so, an warning raised only with the -Xstrict-warings option. In other words, it means that maybe you are doing a logic mistake, but it is up to you to decide.

As others have noticed, in most cases it is not meaningful to catch all exception and you should do something like this:

t.begin()
try {
  doSomething()
  t.commit()
} catch {
  case e: DuplicatedKeyError => ...
  case e: BrokenConnectionError => ...
  case e: DumbInputDetectedError => ...
}

i.e. handle meaningfuly all known error types.

But if you are positive that you want to ignore (or handle the same way) all possible exceptions, then just ignore the warning.

Jeff
  • 953
  • 2
  • 11
  • 21