Add support for slingshot selector syntax when using with-handler

[dparis]

As discussed in #9, full support for slingshot selector syntax would be a nice addition to dire. Until slingshot opens up a (catch+ ...) function, I feel like I did the next best thing. First, I figured that rather than just using class names as keys in the error-handlers meta map, dire could use any type of slingshot selector: vector of key-value pairs, class name, or a predicate function. Then, I created a set of multimethods to match various error handlers to the thrown object. Each multimethod implementation, along with the dispatch, try to follow the logic of slingshot's selector syntax as closely as possible. Since it's not implemented as a macro, full support for a selector form was left out for now, but otherwise every other selector syntax is usuable. If anyone wants to take a stab at supporting full forms, it should just require creating a new multimethod and adding some logic to the dispatch function. As well, by making it modular, if slingshot ever does expose its catch macro logic we can just use the multimethods to pass the value into the slingshot functions. I think this strikes a nice balance between getting the functionality now while still making it painless to hook into the slingshot code if it becomes available.

Using the built-in slingshot support for predicate matching in the try+, I'm calling a HOF to generate a predicate closure around the task context when matching the catch clause. This allows the catch clause to only catch exceptions which match known selectors for that task. Any exception that comes through that isn't matched to a registered error-handler is passed along to the next catch clause in the try+. Right now, this shouldn't happen, since all current with-handler call semantics will be caught by either the pre-/post-condition clauses or the selector matching predicate, but it's nice to know the option exists.

As an example of what can now be done:

(defn div [a b]
  (if (= b 0) 
    (throw+ {:type :div-by-zero :message "You divided by zero, ya turkey!"})
    (/ a b)))

(with-handler!
  #'div
  [:type :div-by-zero]
  (fn [e & args] (println (:message e))))

There are some additional tests in the test suite which handle the supported slingshot syntax cases. All tests, new and old, pass cleanly. Oh, and I updated midje and lein-midje to the current stable versions in order to get :autotest support. Let me know if the code looks good and, if you like it, what an ETA might be for getting it merged upstream. In the mean time, I'm going to likely create a new clojars fork so that I can use the functionality in a project I'm working on.

Cheers!

[dparis]

For the time being, this functionality can be accessed using my clojars fork: https://clojars.org/org.clojars.dparis/dire

Let me know if/when this gets merged into upstream and I'll happily get rid of my fork.

[dparis]

Just wanted to leave some feedback. I've been using the new slingshot syntax within an app I'm working on and it seems to work great so far.

In a processing pipeline where data is passed through numerous chained functions, I've got pre- and post -conditions checking data validity at each pipeline step. In the pre-/post-condition handlers, I'm performing some error validation and constructing an exception map using an error constructor function which contains a context-aware message, an array of the offending data, and the arguments to the pipeline function that caused the failure. I'm then throwing this map using throw+. Finally, I've added handlers to the pipeline wrapper function which can handle specific error types differently, or generically by looking at the type namespace.

The top-level handlers generally log the error, signal the error on an external queue for notification purposes, and allow the daemon which feeds the pipeline to continue processing. All this without having having any exception handling try/catch/throw code or validation code within my application logic, and without any custom exception classes.

Thanks to @MichaelDrogalis and all Dire and Slingshot contributors!

[MichaelDrogalis]

Hiya @dparis!

Wow, this is terrific! Thanks for your effort :)

Give me a couple of days to stew on this. I'll keep you in the loop.

[dparis]

@MichaelDrogalis Sure thing. :smile: Glad I could contribute, and I hope I didn't overlook anything important. If you come up with any additional test cases that fail or need to be fleshed out, I'd be happy to poke at it some more.

Also, FYI, I updated the project group so that I could deploy my fork to clojars. If you do end up wanting to merge this upstream, let me know before you do and I can reset the project settings in the PR if you prefer to do the merge without having to edit anything afterwards.

[MichaelDrogalis]

Will do! Super glad to hear the library is working out for you. :) Thanks again for your help.

[MichaelDrogalis]

Heya @dparis. I gave this a review and found some minor things. I had a cow when I realized that you can catch based on an arbitrary predicate - that's terrific. I'd be happy to merge after the issues are resolved. Thanks so much! :)

[MichaelDrogalis]

Oh, also - would it be possible to change the pull request to merge into develop? If not, I can apply a patch.

[dparis]

@MichaelDrogalis Once the above discussions are resolved, I'll close this PR and open another one against the development branch. Github won't let you change the PR target after it's been created. Bummer!

Since I'll be doing that, I could rebase my commits into a single commit. Would you prefer that, or do you want to maintain the granularity of my individual commits?

[MichaelDrogalis]

You can squash them into one commit if you prefer. It doesn't matter much to me. One other thing - be sure to change the project version to 0.5.0-SNAPSHOT, not 0.5.0. I want to upgrade a few projects before releasing.

Once again - thank you so much for this patch. You are awesome. :D

[MichaelDrogalis]

Took some time this weekend to test the impending changes on other projects. Everything looks great. Tuesday morning seems good for a release.

[dparis]

@MichaelDrogalis Hey, glad it's working out. I had a major deadline come up and I've been really slammed the last few days. I hope to get a chance to review some things before tuesday, but if not I'll get a PR containing at least the above fixes re-submitted against the dev branch by tomorrow evening (PST). I've been using the new branch in an app over the weekend, and I found it mostly works as expected, but there were a couple of issues:

1. I'm not sure yet, but I think I'm seeing a bug related to the non-deterministic way selectors are ordered. Because they can be registered from anywhere, coupled with the fact that the error-handlers meta is storing the registered handlers in a simple hashmap, there's no intuitive way to know in which order they'll be applied. I was considering solving this using a priority-map but I haven't had time to flesh out the exact implementation. In essence, though, you'd ideally be able to call with-handler using the current syntax and get the current behavior, but optionally pass a priority argument which would allow for fine-grained control over the order in which the selectors are matched. Since priority-map essentially also functions as a priority queue, I should be able to keep the same recursive iteration logic and just make the values a map with a priority and function as values rather than just a bare function.
2. There seems to be some unexpected behavior when throwing from within a handler and expecting the error to bubble up from there. This could be because the selection application order issue mentioned above is masking the intended behavior. I don't know yet since I haven't had a chance to really dig into debugging it.

Thankfully, both of those cases should be easy to write tests for, so hopefully it'll be quick fix. If I can't get around to it by Monday, I'm sure you could release the current version with some warning text informing people that selection application order is still non-deterministic and may cause unexpected behavior if a throw object matches multiple selectors within a given selector type.

Cheers!

[MichaelDrogalis]

Howdy @dparis. Consider there to be no hard release deadline if we want to make some tweaks. I only picked that date because it's a good time to get a lot of eyes on a new release. :)

For 1: That's actually intentional. I didn't want ordering to matter, in a similar way to how multimethods works. Sometimes more than one handler can apply, and that's where I stopped short of using something like prefer-method. If it's a problem that you're having in real usage of the library, we can address it. If not, I wouldn't worry.

For 2: Yes, I can help look into it as well.

Again, no rush when we're still making the patch bullet proof. Have a good Sunday!

[dparis]

Regarding 1:

My issue arises from a use case where I'm building a pipeline from composable functions, each of which can throw any number of exceptions. So:

(defn step1 [a]
  ;; May throw any number of exceptions from consumed external libraries
)

(defn step2 [a]
  ;; May throw any number of exceptions from consumed external libraries
)

(defn pipeline [a]
  (-> a
       (step1)
       (step2))

;;
;; I'd like to do something like this...
;;

;; This particular exception is recoverable at the call site with some cleanup
(with-handler! #'step1
  some.specific.Exception
  (fn [e & args] (step1 (clean-up args))))

;; Uh oh, something happened when we foo'ed the bar. Maybe this step performs a
;; lot of calculations using external libraries for net access / machine learning 
;; classification / etc, and I don't have time to read through the docs of every 
;; single library dependency used in order to handle every custom exception. Gotta 
;; catch 'em all!
(with-handler! #'step1
  Throwable
  (fn [e & args] (generate-and-throw+ :step-1-exception e)))

(with-handler! #'step2
  Throwable
  (fn [e & args] (generate-and-throw+ :step-2-exception e)))

;; Let's say any :step-2-exception is always recoverable if we perform 
;; some cleanup and retry the entire pipeline again from the beginning
(with-handler! #'pipeline
  [:type :step-2-exception]
  (fn [e & args] (do (log/info e) (pipeline (cleanup args)))))

;; Caught some kind of unhandled exception here that could have 
;; come from any pipeline step, so log it and die
(with-handler! #'pipeline
  Throwable
  (fn [e & args] (log/fatal e)))

Essentially, I'd like to be able to trap and handle specific exceptions at the individual step level, but in the case of an unpredictable exception at any given step I'd like to throw a step-specific custom exception so that the pipeline handlers can know at which step the unhandled exception came from. Right now, because Throwable will also catch any instances of some.specific.Exception, the handler on step-1 will sometimes apply the incorrect handler depending solely on the order that the selector tries to match them.

If you have any tips on how to implement this more effectively, I'd appreciate any guidance. :smile:

[MichaelDrogalis]

"Gotta catch 'em all" - made me laugh. :P

I see your concern here. I think if we take the time to carefully consider a way to offer optional ordering semantics, it would more closely emulate try/catch - which is good IMO. So this sort of use case seems alright, but let's handle it after this patch goes through. We can collaborate on it.

Regarding how to do it now, the best you have is to catch the most general thing and dispatch on a multimethod. It's actually not a bad approach, especially if everything throws clojure.lang.ExceptionInfo, but we can do better for sure.

By the way, what domain are you writing this pipeline for? Just curious.

[dparis]

Closing in favor of the #17 PR against the develop branch.