core.async

core.async exception rewriting can result in an incorrect return value

Details

  • Type: Defect Defect
  • Status: In Progress In Progress
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test

Description

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)))))

Activity

Hide
Steve Smith added a comment -

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

Show
Steve Smith added a comment - 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
Hide
Kevin Downey added a comment -

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.

Show
Kevin Downey added a comment - 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.

People

Vote (1)
Watch (1)

Dates

  • Created:
    Updated: