Catching all exceptions: unexpected behavior

[jwr]

I'm not actually sure if this is an issue or a question, but it certainly was unexpected and caused problems in my existing software, so here goes:

We went through our code recently and modified most try/catch blocks to try+/catch, using additional functionality provided by slingshot. So far so good. Problem is, we often had a (catch Exception e...) clause at the end, that was supposed to catch pretty much everything that could happen in that block and perform cleanups.

Now, things were fine as long as exceptions were thrown by java code. But then it turned out that clj-http throws its unexpected-status exceptions (like "403 forbidden", see wrap-exceptions in client.clj in clj-http) using throw+. And what gets passed to throw+ is a response object (a map) and a string.

The result was that our code worked fine as long as the block around clj-http used (try/catch), but completely missed the exception when try was changed to try+, because slingshot would "unpack" the object that was thrown, find a map, and try to match that. And (catch Exception e...) would not match a map.

To demonstrate the problem:

(try (http-request {:url "http://fablo-data-production-eu-west-1.s3.amazonaws.com/no-permission" :method :get}) (catch Exception e (prn "got it" (.toString e))))
"got it" "slingshot.ExceptionInfo: clj-http: status 403"

-- this is fine and what I would expect.

(try+ (http-request {:url "http://fablo-data-production-eu-west-1.s3.amazonaws.com/no-permission" :method :get}) (catch Exception e (prn "got it" (.toString e))))

this does not get caught and SLIME catches the exception: clj-http: status 403 [Thrown class slingshot.ExceptionInfo], which I think is a problem.

There are actually several problems here.

1. I'm not at all sure if library code (like clj-http) should use throw+ outside a try+ block.
2. I would expect (catch Exception e) to catch every exception. If it does not, it means that try+ is not a replacement for try and one should be very, very careful using it.
3. I need to have a way to catch everything, and I'm not sure if (catch (constantly true)) is the best idea.

As a temporary workaround I'll hunt down my try+ blocks and introduce (catch (constantly true)) or (catch Object), but I wanted to bring it up and see what you think.

[scgilardi]

Initial thoughts: (catch Object o ...) is the try+ equivalent of (catch Exception e ...): "catch every thrown thing". Once inside the (catch Object o ...) handler you can decide how you want to act based the value or type of o.

1. I think if a library documents what it throws on exceptions, calling throw+ outside a try+ block is fine. throw+ does throw a A RuntimeException and is interoperable with java or clojure try in that way. There are helper functions in the slingshot namespace for retrieving the context or thrown object from such an exception.
2. There's no way for (catch Exception e) to catch every exception within a try+. throw+ throws objects that may or may not be Exceptions.
3. (catch Object o ...) is the correct way to catch everything.

[jwr]

I guess each of your points makes sense, I was just hit by an unfortunate combination here. I modified our code to (catch Object o ...) for now, which means I have to start making use of &throw-context. Formerly my code would do (.printStackTrace e) and (.getMessage e).

I don't think this is wrong, but I still feel this is a trap for the unwary. I think at the very least it merits a mention in the slingshot README. My takeaway is: try+ is not "100% compatible with Clojure and Java's native try and throw both in source code and at runtime". Your (catch Exception) clauses will not catch all exceptions. They need to be modified to (catch Object o) and use &throw-context to get at the stacktrace.

The twist here is that you will only run into problems when something down the call chain uses throw+ :-)

[scgilardi]

try+ (catch Exception) clauses will catch all exceptions. They won't catch non-Throwable objects thrown by throw+. I agree that this warrants a section in the README. What you may want here is:

(try+
  [...]
  (catch Exception e ...)
  (catch Object o ...))

The Object clause will then catch only objects not derived from Exception. (this may have the unwanted effect of catching non-Exception Throwables. If that's a problem, another catch clause can fix it up.

[amalloy]

Just wandering in to note that I got bitten by this just now. Reading over the discussion I think I agree that it can't be any other way, but it is not at all intuitive - I imagined that (catch SomeClass x) was checking the class of the thrown exception, rather than whatever slingshot's idea of a throw-context is.

So I guess I don't really agree with the statement that (catch Exception) will catch all exceptions - it catches everything that slingshot considers to be a thrown exception, but not what slingshot considers to be other thrown things; thinking instead at the JVM or Java level, there are things which are unarguably exceptions which are not being caught. Some clarification of this in the readme is probably all that is needed.

[blendmaster]

Why not add a special syntax for catching anything? I propose

(try+
   (stuff)
(catch :anything _
   (println "caught a thrown+ object or an Exception!"))
(catch :anything-throwable _
   (println "caught a thrown+ object or something Throwable,
               maybe even OutOfMemoryError! gross!")))

It's a lot less weird looking than (catch Object e), and avoids the necessity of having to rethrow or otherwise filter nasty Throwables like ThreadDeath when you don't really want to catch them.

I guess a symbol like :anything could technically be misinterpreted as a predicate (map access) catch form, but I feel like nobody actually uses a symbol in that case.

Edit: Something to catch any thrown+ objects without catching Exceptions would also be nice, maybe:

(try+
   (stuff)
(catch :non-exceptions _
   (println "caught a thrown+ object, but not an Exception!")))

[scgilardi]

sorry for the outrageously long delay. anyone who is still interested, please review: #46