Issue 275: Alternative approach using a listener #447

WilliamParker · 2020-01-13T00:40:59Z

I gave the previous PR #425 some thought and on reflection, I think this can be done in a simpler way that doesn't impact as much of the core code with a listener. The actual behaviour upon reaching the limit could then be easily user-defined, probably with some default options supplied.

Thoughts? There is definitely some stuff to clean up here before merging but I wanted to get thoughts on the general approach before getting too far into this. The tests are fundamentally the same as in #425 with some minor adjustments to invoke the listener; in a final PR I'd change them further to not solely rely on exceptions being thrown, which would allow them to be cross-platform.

@EthanEChristian @mrrodriguez

mrrodriguez · 2020-01-13T15:24:30Z

src/main/clojure/clara/rules/listener.cljc

@@ -23,6 +23,7 @@
  (remove-activations! [listener node activations])
  (fire-activation! [listener activation resulting-operations])
  (fire-rules! [listener node])
+  (activation-group-transition! [listener original-group new-group])


Any new functions added here are actually breaking changes to user-provided listeners since they will not be implementing these new functions and they will be called. That'll likely result on the JVM as a AbstractMethodError. This isn't an issue to be solved in this particular work. I'm just raising the general concern since it is applicable.

Aside: I have always been bothered with how this is what happens in Clojure when you add protocol functions. Even if you have Object type defaults for all functions, they'll be ignored and not inherited by individual protocol implementors on things like records.

It could be argued that you aren't supposed to define huge protocols in clj and that's where we are going wrong. However, even if this was split across N many protocols, we still have hardcoded assumptions in the engine that we can call every function them on the underlying implementations provided. However, more granularity could let implementors impl a smaller protocol and all the rest would be a default with an default eg. Object impl, so that sort of still works out.

Eg. we could have multiple protocols like:

IAlphaEventListener IAccumEventListener IActivationEventListener IFireEventListener <... etc ...>

I think an option that seems promising to alleviate this in the future is to wrap calls to user-provided listeners with a listener that Clara controls. This listener can catch the errors for unimplemented functions by the underlying listener. Constantly handling exceptions would not be ideal from a performance perspective, so this failure-case would likely need to be cached and remembered for subsequent calls. We could warn upon session creation - by checking all functions immediately - to assist in people upgrading. Or we could warn on first time we encounter failed call, however, that is less deterministic.

I'd have to double check, but we may be using the DelegatingListener already over user-supplied listeners, so it may already be in the position to do this.

I agree with the premise here, we've had breaking changes to listeners in the past and while not a big deal it still impacts consumers.

This isn't an issue to be solved in this particular work.

It might be worth it to log an issue to look into ways that we can change listeners in the future to be less impactful. For now i think i am ok with adding a new method to this protocol, and announcing it as a breaking change, like we did in the past.

I've logged #457 to look into a better approach to this with a thought on how to do it. For now I think we can do as Ethan suggested above.

mrrodriguez · 2020-01-13T15:45:11Z

src/main/clojure/clara/tools/loop_detector.cljc

+  (:require [clara.rules.listener :as l]
+            [clara.rules.engine :as eng]))
+
+(deftype LoopDetectorListener [cycles-count max-cycles fn-on-limit]


It relates to my comment @ https://github.com/cerner/clara-rules/pull/447/files#diff-a498c1d1444fc426bcaddeaa009e8dbfR26

It is clear it is tedious to setup a listener that implements all the functions that could be needed by any listener at a given Clara version.

mrrodriguez · 2020-01-13T15:49:55Z

src/main/clojure/clara/tools/loop_detector.cljc

+  (:require [clara.rules.listener :as l]
+            [clara.rules.engine :as eng]))
+
+(deftype LoopDetectorListener [cycles-count max-cycles fn-on-limit]


fn-on-limit is named backwards to what to I expect - ie. on-limit-fn

This is certainly more regular when it comes to cljs-land and web events - but I think it has good precedent in general. It's typically shortened to just on-limit, but it isn't bad to say "fn" to make it clear what it is.

to be more explicit it could also be said that on-limit-exceeded-fn would be more clear.

mrrodriguez · 2020-01-13T15:50:24Z

src/main/clojure/clara/tools/loop_detector.cljc

+  (:require [clara.rules.listener :as l]
+            [clara.rules.engine :as eng]))
+
+(deftype LoopDetectorListener [cycles-count max-cycles fn-on-limit]


Should it just be called MaxCycleListener to be more explicit?

MaxCycleListener seems a bit vague from a new user perspective, CyclicalRuleListener perhaps...

+1 I like "CyclicalRuleListener", will change.

mrrodriguez · 2020-01-13T15:51:03Z

src/main/clojure/clara/tools/loop_detector.cljc

+  (to-transient [listener]
+    listener))
+
+(defn with-loop-detection [session fn-on-limit max-cycles]


Why do the arguments come in a different order for the fn-on-limit & max-cycles here than what they come in for the actual record constructor?
I'd prefer consistency. It seems more (subjectively) natural to have the fn arg come last in both cases.

Agreed, I have changed this in #458

mrrodriguez · 2020-01-13T15:55:19Z

src/main/clojure/clara/tools/testing_utils.cljc

+     "Given a Throwable, return in order the ExceptionInfo data maps for all items in the
+      chain that implement IExceptionInfo and have nonempty data maps."
+     [t]
+     (let [throwables  ((fn append-self


Can we just make it a let-binding instead of a recursive anonymous inline fn? It's just making the impl look harder to follow.

(let [append-self (fn append-self [prior t1]] ...)] ...)

Or use letfn for it to avoid repeating the name if that was the issue:

#?(:clj (defn ex-data-maps "Given a Throwable, return in order the ExceptionInfo data maps for all items in the chain that implement IExceptionInfo and have nonempty data maps." [t] (letfn [(append-self [prior t1] (if t1 (append-self (conj prior t1) (.getCause ^Throwable t1)) prior))]) (->> t (append-self []) (into [] (comp (map ex-data))))))

+1, changed in #458

mrrodriguez · 2020-01-13T15:59:30Z

src/main/clojure/clara/tools/testing_utils.cljc

@@ -203,3 +203,19 @@
                     e# \newline
                     "Non matches found: " \newline
                     res#)))))))
+
+#?(:clj
+   (defn ex-data-maps


"in order" is a bit vague. I'd immediately ask "what order?" and "ascending or descending?"

Given a throwable/exception/error `t`, return all non-empty `ex-data` maps from stack trace cause chain in the order they occur traversing the chain from this `t` on up the rest of the call stack.

I also tried to make it more cljs friendly since there isn't a reason this has to stay stuck in clj-only for long. It wouldn't require much change to handle both.

I've used a slightly modified version of this docstring in #458

Note that I think this can return empty ex-data maps; I have changed the docstring you proposed above accordingly.

mrrodriguez · 2020-01-13T16:02:42Z

src/test/clojure/clara/test_infinite_loops.clj

+;;           (is (= (-> loop-data first  :listeners count) 1)
+;;               "There should only be one listener.")
+;;           (is (-> loop-data first  :listeners ^PersistentTracingListener first .-trace not-empty)
+;;               "There should be tracing data available when a traced session throws an exception on an infinite loop."))))))


This is still an issue with the listener approach?
I'm not sure there necessarily should/can be a trace available when arbitrary exceptions are thrown during rule evaluation.

If we want that, I think that's a broader issue than just specifically having a trace when this max cycle logic throws.

If there is an infinite loop I think it is useful to able to use a trace to show which rules are looping. My approach in #458 allows the trace to be added.

mrrodriguez

I do like the transition to listener approach for the most part - beyond my concerns with listener function additions being breaking changes - as I discuss in the review @ #447 (comment)

WilliamParker · 2020-06-29T22:14:42Z

I had some difficulties with Git merge conflicts when trying to add changes on this PR, so I have created a new PR branched off master at #458 that addresses the comments in this PR.

Closing in favour of that PR.

WilliamParker added 2 commits January 12, 2020 23:36

Issue 275: Draft of approach using a listener

ed52bef

Add method to inspection listener

ced6a3e

WilliamParker changed the title ~~Issue275 listener draft~~ Issue 275: Alternative approach using a listener Jan 13, 2020

mrrodriguez assigned mrrodriguez and unassigned mrrodriguez Jan 13, 2020

mrrodriguez requested review from EthanEChristian and mrrodriguez January 13, 2020 15:19

mrrodriguez assigned WilliamParker Jan 13, 2020

mrrodriguez added the error-reporting label Jan 13, 2020

mrrodriguez reviewed Jan 13, 2020

View reviewed changes

mrrodriguez requested changes Jan 13, 2020

View reviewed changes

WilliamParker mentioned this pull request Jun 29, 2020

Issue 275: Thow exception on infinite loops #425

Closed

WilliamParker closed this Jun 29, 2020

WilliamParker mentioned this pull request Aug 24, 2020

Issue 275: Max-cycles listener using a listener-based approach #458

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 275: Alternative approach using a listener #447

Issue 275: Alternative approach using a listener #447

WilliamParker commented Jan 13, 2020 •

edited

Loading

mrrodriguez Jan 13, 2020

EthanEChristian Mar 15, 2020 •

edited

Loading

WilliamParker Jun 17, 2020

mrrodriguez Jan 13, 2020

mrrodriguez Jan 13, 2020

EthanEChristian Mar 15, 2020

mrrodriguez Jan 13, 2020

EthanEChristian Mar 15, 2020

WilliamParker Jun 17, 2020

mrrodriguez Jan 13, 2020

WilliamParker Jun 29, 2020

mrrodriguez Jan 13, 2020

WilliamParker Jun 29, 2020

mrrodriguez Jan 13, 2020

WilliamParker Jun 29, 2020

mrrodriguez Jan 13, 2020

WilliamParker Jun 29, 2020

mrrodriguez left a comment •

edited

Loading

WilliamParker commented Jun 29, 2020

Issue 275: Alternative approach using a listener #447

Issue 275: Alternative approach using a listener #447

Conversation

WilliamParker commented Jan 13, 2020 • edited Loading

Choose a reason for hiding this comment

EthanEChristian Mar 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrrodriguez left a comment • edited Loading

Choose a reason for hiding this comment

WilliamParker commented Jun 29, 2020

WilliamParker commented Jan 13, 2020 •

edited

Loading

EthanEChristian Mar 15, 2020 •

edited

Loading

mrrodriguez left a comment •

edited

Loading