Welcome! Please see the About page for a little more info on how this works.

0 votes
in core.async by
The following test case passes in core.async 0.2.395, but fails on 0.3.443 due to an incorrect return value from an exception catch. See comments in the test for details.


(ns core-async-exception-test
  (:require [clojure.test :refer :all]
            [clojure.tools.logging :refer [log]]
            [clojure.core.async  :as a]))

(deftest async-error-proof
  (let [resp (a/go
               (try
                 ;; In version 0.2.395 of core.async this test passes
                 ;; (i.e. the behaviour would be as expected). However
                 ;; in 0.3.443 (and other 0.3.x versions) this
                 ;; fails.
                 ;;
                 ;; Note that we wrap the :outer-ok return
                 ;; value. However, with this code-structure in place
                 ;; an *unwrapped* version of the exception is thrown,
                 ;; causing an UnsupportedOperationexception when it
                 ;; attempts to destructure it.
                 ;;
                 ;; Any of the following changes will fix the issue:
                 ;; * Remove the empty finally.
                 ;; * Unwrap the :outer-ok and remove destructuring
                 ;; * Remove the 'value' go/fetch.

                 (let [[r] (try

                             (let [value (a/<!
                                          (a/go :any))]

                               (log :info "Got value, throwing exception")
                               (throw (ex-info "Exception" {:type :inner})))

                             (catch Throwable e
                               (log :info "Caught the exception")

                               [:outer-ok])

                             (finally))] ;; <- Remove this finally to make it work



                   (throw (ex-info "Throwing outer exception" {:type :outer})))

                 (catch clojure.lang.ExceptionInfo ex
                   (assert (= :outer (-> ex ex-data :type)))
                   (log :info "Got Exception Info")
                   :ok)

                 (catch UnsupportedOperationException ex
                   (log :info "Got unsupported exception")
                   :unsupported)))]

    (is (= :ok (a/<!! resp)))))

5 Answers

0 votes
by

Comment made by: tarkasteve

Note, that a bisect on the history shows this behaviour is associated with the changes for ASYNC-169: https://github.com/clojure/core.async/commit/a879b388a47786ef8ee21458eb45403193028f49

0 votes
by

Comment made by: hiredman

a brief description of exception handling:

a try/catch/finally has 3 parts with those names control flow ends up something like

try: do stuff and on exception go to catch if it exists, or else finally if it exists goto finally catch: do stuff and on exception go to finally if it exists goto finally finally: do stuff if came here via exception throw exception

some details about exception handling in go blocks:

blocks that should be jumped to in the case of an exception are pushed
on to EXCEPTION-FRAMES

when the try block executes it first pushes the
finally block if it exists and then the catch block (as of now a catch
block always exists even if there are no catch clauses in the
expression). when execution of the try block ends normally it jumps to
the finally block. if an exception is raised in jumps to the block at
the top of EXCEPTION-FRAMES.

discussion of the actual bug:

The bug here had to do with management of pushing and popping of
EXCEPTION-FRAMES. Before this patch the loop around running the state
machine would catch all exceptions, then jump to the block at the top
of EXCEPTION-FRAMES and also pop the block off the top of
EXCEPTION-FRAMES. This was a problem because depending on the
execution path, there are either 1 or 2 frames to pop (either a catch,
or a catch and a finally).

This table shows the problem:

| expr | frames pushed by try | frames popped by try | frames popped by catch | frames popped by finally | frames popped by state machine | sum ex | sum no ex | |------------------------+----------------------+----------------------+------------------------+--------------------------+--------------------------------+--------+-----------| | old try/finally | 2 | 2 | 0 | 0 | 1 | 1 | 0 | | old try/catch/finally | 2 | 2 | 0 | 0 | 1 | 1 | 0 | | new try/finally | 2 | 1 | 1 | 1 | 0 | 0 | 0 | | new try/catch/finally | 2 | 1 | 1 | 1 | 0 | 0 | 0 |

the sums are pushs - pops, for the no exception case
the only pops are the frames popped by the try and the finally, for
the exception case the pops are the pops from the catch, the finally,
and the state machine loop.

This patch removes the implicit pop of the exception handler from the
loop running the state machine and replaces it with explicit popping
instructions. The try block pops off one frame on successful execution
and them jumps to the finally if it exists. The finally block pops off
one frame. The catch block pops off one frame.

0 votes
by
_Comment made by: alesguzik_

I have encountered a very similar case, and it is broken even with the patch above:


(deftest ASYNC-198-2
  (let [resp (runner
               (try
                 (try
                   (throw (ex-info "Exception" {:type :inner}))
                   (pause :any)
                   (catch clojure.lang.ExceptionInfo ex
                     (throw (ex-info "Throwing from the catch" {:type :nested})))
                   (finally)) ;; <- Remove this finally to make it work
                 (catch Throwable e :ok)))]
    (is (= :ok resp))))


The result is

FAIL in (ASYNC-198-2) (ioc_macros_test.clj:536)
expected: (= :ok resp)
  actual: (not (= :ok #error {
 :cause "Throwing from the catch"
 :data {:type :nested}
 ...


It doesn't matter whether I catch clojure.lang.ExceptionInfo or Throwable in any of those places in the test.
0 votes
by

Comment made by: hiredman

I suspect the above failing test is the same as ASYNC-220 which has a patch which builds on this one

0 votes
by
Reference: https://clojure.atlassian.net/browse/ASYNC-198 (reported by alex+import)
...