exception data lost in multi-threaded environment

[jeroenvandijk]

version

org.babashka/sci 0.7.39 (and older)

platform

JVM

problem

Exception data is lost* when a function that was created in another thread is invoked and throws.

(*) most importantly the parts of ex-data that hold the :line and :column data of the relevant sci code.

repro

;; helper
(defn invoke-ex-fn [f]
  (try
    {:type :ok
     :value (f)}
    (catch Throwable e
      (merge {:type (class e)} (select-keys (ex-data e) [:type :line :column])))))

;; Invoke problematic code from different threads and get a different result
(let [f (sci.core/eval-string "(fn [] (/ 1 0))")]
  [(invoke-ex-fn f)
   @(future (invoke-ex-fn f))
  (invoke-ex-fn f)])
 ;=>
'[{:type :sci/error, :line 1, :column 8} 
  {:type java.lang.ArithmeticException} ;; => result from different thread
  {:type :sci/error, :line 1, :column 8}]

expected behavior

I need the error information to display a more helpful error message (like in Babashka with a line pointing to the problematic code). When the problematic code was invoked from a different thread this doesn't work. I'm using Sci in a multithreaded environment (a web server) so I've seen this scenario frequently now. It took me some time to identify it.

(Sponsor, though nothing blocking, but definitely nice to have)

[borkdude]

@jeroenvandijk This is related to this:

https://github.com/babashka/sci/blame/master/src/sci/impl/utils.cljc#L97

I'll see if there is a better solution. I'm probably going to rewrite some of the error / stack logic soon, as part of #871.

[borkdude]

TBH, I don't recall why I did this. Perhaps some thread-unsafety.. but I don't see any failing tests. I'll think some more and then will remove the :main-thread-id condition if it's not a problem.

[jeroenvandijk]

Ah I'm happy you were able to already identify the cause. I've been staring at the code to see if I could find it, but had no idea.

[borkdude]

@jeroenvandijk Hmm, I think I found the reason for this change. When I remove this check, I get a failing library test in bb:

Testing qbits.auspex-test

FAIL in (error-test) (/Users/borkdude/.gitlibs/libs/cc.qbits/auspex/1a9d7427e60e1a434a764aa820d1c53f7e22504a/test/qbits/auspex_test.clj:71)
expected: (thrown? ExecutionException (clojure.core/deref (-> (a/future (fn [] (throw (Exception. "meh"))) executor) (a/catch ExceptionInfo (fn [_] :qbits.auspex-test/foo)))))
  actual: nil

Ran 11 tests containing 74 assertions.
1 failures, 0 errors.
{:test 11, :pass 73, :fail 1, :error 0}

[borkdude]

I reverted this solution, but will try to find another one.

[borkdude]

Note to self:

Repro:

(require '[qbits.auspex :as a])

(def executor clojure.lang.Agent/pooledExecutor)

(try @(-> (a/future (fn [] (throw (Exception. "meh")))
                    executor)
          (a/catch clojure.lang.ExceptionInfo (fn [_] ::foo)))
     (catch Exception e (prn (type e))))

This should print: java.util.concurrent.ExecutionException

[borkdude]

Smaller repro:

(def f (future (throw (IllegalArgumentException. "meh"))))

(try @f
     (catch Throwable e
       (prn (type (ex-cause e)))))

This should print IllegalArgumentException and not clojure.lang.ExceptionInfo

[borkdude]

@jeroenvandijk You can preserve location information in futures now with:

#?(:clj
   (deftest thread-test
     (let [invoke-ex-fn
           (fn [f]
             (try
               (f)
               (catch Throwable e
                 (ex-data e))))]
       (let [f (sci.core/eval-string "(fn [] (try (/ 1 0) (catch ^:sci/error Exception e (throw e))))")]
         (is (let [res @(future (invoke-ex-fn f))]
               (is (= {:line 1 :column 13} (select-keys res [:line :column])))))))))

So wrap your function in a try/catch and catch the exception with ^:sci/error on the Exception symbol. And re-throw that exception.

[jeroenvandijk]

@borkdude Very cool, thank you! I will play around with it. FYI, I am not using future specifically, but it was an easy way to reproduce the multithreaded scenario. I believe that what you are suggesting will work for me though! Thanks again!

[borkdude]

Yes, it will work regardless of future I think

[jeroenvandijk]

I need to do more further testing, but by wrapping the body of my external macros in a with-sci-error helper I get what I want in simple scenarios.

(sci/eval-string "..."
                 {:bindings
                  {'with-sci-error (sci/eval-string "(fn [f] (try (f) (catch ^:sci/error Exception ex (throw ex))))")}})