riemann/riemann

riemann.streams/call-rescue is catching java.lang.Error

Closed this issue · 14 comments

yaxum62 commented

call-rescue catches and handles java errors like OutOfMemoryError. This will put JVM in unpredictable state.

mcorbin commented

@aphyr Could we replace Throwable by Exception in call-rescue ? Throwable is also used in pool.clj, time.clj, and some transports.

boernd commented

There was a reason for this change

commit a0a4f943d87748a485c0ed6c08d70656144276df
[...]
Date:   Mon Aug 26 12:26:48 2013 +0200

    Catch Throwable, not only Exception.
    
    Catching only Exception was to narrow.
    
    User could provide code that would result in Throwable,
    like: (swap a (atom {}))
    
    That error would go unnoticed.

diff --git a/src/riemann/streams.clj b/src/riemann/streams.clj
index 5427446..01732e3 100644
--- a/src/riemann/streams.clj
+++ b/src/riemann/streams.clj
@@ -52,7 +52,7 @@
      (doseq [child# ~children]
        (try
          (child# ~event)
-         (catch Exception e#
+         (catch Throwable e#
            (warn e# (str child# " threw"))
            (if-let [ex-stream# *exception-stream*]
              (ex-stream# (exception->event e#))))))
yaxum62 commented

It is certain some throwable should not be handled. Could we at least ignore a subset of throwables like errors in java.lang.VirtualMachineError? Any catch without re-throw of those errors would be evil.

mcorbin commented

I agree. I will try to take a look this week.

mcorbin commented

Something like (swap a (atom {})) will produce a clojure.lang.Compiler$CompilerException, so no need to use Throwable.
I think we could replace Throwable by Exception like it was before. @pyr or @aphyr do you have any advice ?

jamtur01 commented

Are we sure Exception will catch all? Isn't Error NOT a subset of Exception? Won't this miss some error classes?

mcorbin commented

Yes, Error is not a subset of Exception. But to me, we don't want to catch Error (because errors are generally unrecoverable, like OutOfMemoryError).
Maybe i'm wrong, but i cannot think of an example where Riemann needs to catch an Error. If so, we should catch this specific Error (and only this one).

aphyr commented

Note that assertion failures are Errors, not Exceptions.

yaxum62 commented

If there is a good reason to do catch AssertionError, we should catch AssertionError ONLY but not entire Error since there are definitely more reasons to not catch other Errors. Please consider using (catch Exception ...)(catch AssertError ...) instead of (catch Throwable ...)

mcorbin commented

#842 (merged) replace (catch Throwable) by (catch Exception).

Regarding the assert problem, i think we should catch AssertionError right after the assert and rethrow an Exception, something like:

(try (assert (= 1 2)) 
  (catch AssertionError e (throw (Exception. e))))
aphyr commented
mcorbin commented

Yeah, that was a stupid idea :s

jamtur01 commented

@mcorbin So any update to current state from this discussion?

mcorbin commented