taoensso / carmine

Redis client + message queue for Clojure
https://www.taoensso.com/carmine
Eclipse Public License 1.0
1.15k stars 130 forks source link

wcar silently catches exceptions #237

Closed aphyr closed 4 years ago

aphyr commented 4 years ago

I was surprised to realize that wcar catches any exceptions thrown, and returns them as a part of the result vector. I understand why you might want this from a low-level primitive (say, for extracting results of partial computation before the exception was thrown), but... it feels like unexpectedly dangerous behavior for a high-level API entry point. Consider what happens if a user writes:

(wcar {...} (set "foo" 1)
(wcar {...} (set "bar" 2)

... assuming that the set of bar should occur only after the set of foo. If the set of foo fails, wcar swallows that exception and proceeds to write bar, which could invalidate intended redis state. It's... possible that everyone already knows this and is writing defensive wrappers to detect and re-throw exceptions from wcar, but might I suggest documenting this behavior so that new users don't find out the hard way?

aphyr commented 4 years ago

Okay, something else weird is going on here. Not sure this is real; closing for now.

aphyr commented 4 years ago

Ohhhhhhhhh okay. I... think I get it. I was returning a vector (via mapv) from my wcar body, which I assumed meant that it wouldn't engage in the pipelining error-trap-and-throw behavior. What I didn't understand was that wcar doesn't actually return your body, though the docstring examples led me to believe it would! Instead, it evaluates body for side effects, throws away the return value, and returns a vector of redis side effects--and the result type, and throw behavior, are based on how many side effects you scheduled, and whether the exception is... a Redis error, vs something else?

; This works how you'd expect, but only by accident
=> (car/wcar r [(car/set "foo" 1) (car/get "foo")])
["OK" "1"]

; Body is discarded; only side effects are returned
=> (car/wcar r [:hello (car/set "foo" 1) (car/get "foo") :there])
["OK" "1"]

; So in fact, one should call
=> (car/wcar r (car/set "foo" 1) (car/get "foo"))
["OK" "1"]

; Throwing an exception in the body throws like you'd expect
=> (car/wcar r (car/set "foo" 1) (car/get "foo") (throw (RuntimeException. "oh no")))
Execution error at eval9503$fn$fn (form-init8390319469626291680.clj:1).
oh no

; ... Unless, of course, the exception is thrown by something in Carmine, in which case you get a vector of results, including a caught error
=> (car/wcar r (car/set "foo" 1) (car/rpush "foo" 2) (car/get "foo"))
["OK" #error {
 :cause "WRONGTYPE Operation against a key holding the wrong kind of value"
 :data {:prefix :wrongtype}
 :via
 [{:type clojure.lang.ExceptionInfo
   :message "WRONGTYPE Operation against a key holding the wrong kind of value"
   :data {:prefix :wrongtype}
   :at [taoensso.carmine.protocol$get_unparsed_reply invokeStatic "protocol.clj" 130]}]
 :trace
 [[taoensso.carmine.protocol$get_unparsed_reply invokeStatic "protocol.clj" 130]
  [taoensso.carmine.protocol$get_unparsed_reply invoke "protocol.clj" 106]
  ...
  [java.lang.Thread run "Thread.java" 745]]} "1"]

; Unless you only made one request, in which case it's thrown like you'd expect
=> (car/wcar r (car/rpush "foo" 2))
Execution error (ExceptionInfo) at taoensso.carmine.protocol/get-unparsed-reply (protocol.clj:130).
WRONGTYPE Operation against a key holding the wrong kind of value