Closed leonoel closed 2 years ago
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
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."
@bsless BTW any reason why overriding fillInStackTrace
instead of calling the constructor with writableStackTrace
set to false
?
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
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.
Additional thoughts :
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 :
Cancelled
that inherits Throwable
directly with writableStackTrace
disabled.Cancelled
that doesn't inherit Error
to prevent stack trace building. cancelled
to create a new cancellation error, with an optional argument for the message.on-cancel
desugaring to (try ,,, (catch Cancelled _ ,,,))
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))))
(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 :
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))))))
Fixed in b.25
Currently when a process is cancelled before termination it throws a new instance of
ExceptionInfo
with an entry for key:cancelled
. Two problems :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 :
Checking could be made easier with simple sugar :