requirements for cancellation errors

[leonoel]

Currently when a process is cancelled before termination it throws a new instance of ExceptionInfo with an entry for key :cancelled. Two problems :

• possible conflicts with user-crafted exceptions.
• suboptimal performance, because it may happen frequently and each time the exception is reconstructed with the entire stacktrace.

The only requirement of the cancellation error is to be able to disambiguate it from other errors. A singleton would be a better fit - no runtime costs, fast checking based on identity.

The singleton is immutable and can be exposed :

(def cancelled "The object thrown by processes cancelled before termination." (comment TODO))

Checking could be made easier with simple sugar :

(defmacro on-cancel "
Evaluates body and returns result of last expression.
If evaluation fails due to cancellation, evaluates recovery expression and returns its result.
" [recovery & body]
  `(try ~@body
     (catch ~(if (:js-globals &env) :default Throwable) e#
       (if (identical? e# cancelled) ~recovery (throw e#)))))

[bsless]

Another possibility to consider if using errors for cancellation but still not wanting to lose the message is a stackless exception
https://clojureverse.org/t/exceptions-for-control-flow/5874/3?u=bsless

[leonoel]

Yes, IMO the stacktrace is never useful so we can use that trick (singleton or not).

Do you have a use case in mind where the cancellation error should have a payload or a specific message, which would disqualify the singleton approach ? In my experience, when a process is cancelled the error can be propagated from a deeply nested process so the specificity of the message is not really meaningful, I would find it less confusing to read a more generic message such as "Process cancelled."

[leonoel]

@bsless BTW any reason why overriding fillInStackTrace instead of calling the constructor with writableStackTrace set to false ?

[bsless]

You might want specific cancelation errors to know exactly where or what got canceled.
Regarding the implementation, it was about a year ago, I don't recall, exactly, but I think I had an issue there.
Another alternative impl is in a closed PR in malli https://github.com/metosin/malli/pull/441/files

[leonoel]

Well if you need to subclass ExceptionInfo, then you don't have access to the Throwable constructor so overriding the method makes sense indeed.

However I'm not sure there's a need for subclassing.

[leonoel]

Additional thoughts :

• a new class is necessary to prevent stack trace construction, no matter how
• if there's a specific class for cancellation errors, it can also carry a message for humans
• class checking is cheap, so it can be used to discriminate cancellation errors

So it's pretty clear that cancellation errors should have a dedicated class, but now what about the class hierarchy ?

In my experience, cancelled status is different enough from other errors to not inherit java.lang.Exception. Usually when you want to recover from cancellation, you want to recover only from that, and the opposite is also true.

The plan would be :

• (private, CLJ) define a new class Cancelled that inherits Throwable directly with writableStackTrace disabled.
• (private, CLJS) define a new type Cancelled that doesn't inherit Error to prevent stack trace building.
• (public API) define a function cancelled to create a new cancellation error, with an optional argument for the message.
• (public API) define a macro on-cancel desugaring to (try ,,, (catch Cancelled _ ,,,))

[mjmeintjes]

Not sure if this is relevant to this issue:

I have found it pretty difficult to reliably determine whether an thrown exception was due to a task being cancelled or due to a real exceptional circumstance.

For example, if a thread is cancelled it throws an java.lang.InterruptedException:

  (ms/?
   (ms/sp
    (ms/? (ms/via ms/cpu (Thread/sleep 2000)))))

When datomic is interrupted, it throws an ExceptionInfo, and you have to check the ex-causes for the actual interruped exception.

Would this proposal include wrapping InterruptedException (and other similar exceptions)?

I've been using the following code to determine whether an exception is caused by cancellation:

(defn ex-causes [ex]
  (->> ex
       (iterate #(ex-cause %))
       (take-while some?)
       (take 100)))

(defn interrupted-exception-type? [exs]
  (boolean
   (not-empty
    (set/intersection
     #{java.io.InterruptedIOException
       java.lang.InterruptedException
       java.util.concurrent.RejectedExecutionException
       java.nio.channels.ClosedByInterruptException}
     (set (map type exs))))))

(defn- cancelled-exception? [exs]
  (some (fn [ex]
          (let [{:keys [:cancelled :cancelled?]} (ex-data ex)]
            (or cancelled cancelled?)))
        exs))

(defn- exceptions [ex]
  (set (concat (ex-causes ex)
               [ex])))

(defn ex-cancelled? [e]
  (let [exs (exceptions e)]
    (or (cancelled-exception? exs)
        (interrupted-exception-type? exs))))

[leonoel]

(public API) define a function cancelled to create a new cancellation error, with an optional argument for the message.
(public API) define a macro on-cancel desugaring to (try ,,, (catch Cancelled _ ,,,))

After more thinking, this is overengineering. Clojure has good support for classes, better expose the class with a public constructor and check with try/catch or instance?. The only downside is it doesn't work with namespace aliases, but (:import missionary.Cancelled) works well in both clj and cljs.

@mjmeintjes thank you for raising this concern. I'm not interested in solving the "catch everything that could be an interruption" problem, because :

• it's not zero-cost
• it's not future-proof
• it can't be complete (other libraries can implement their own mechanism)

I think it's fine to expect the user to solve this on a case-by-case basis. If consistency is required, writing an adapter is easy enough :

(require '[missionary.core :as m])
(import missionary.Cancelled)

(def sleeper
  (m/via m/blk
    (try (Thread/sleep 2000)
         (catch InterruptedException e
           (throw (Cancelled. (.getMessage e)))))))

(def datomic
  (m/via m/blk
    (try (d/q '[,,,])
         (catch ExceptionInfo e
           (throw (if (ex-cancelled? e)
                    (Cancelled. (.getMessage e)) e))))))

[leonoel]

Fixed in b.25