Fix RuleOrderedActivation Fressian Handler #395
Conversation
mmotter
commented
Jun 7, 2018
mmotter
commented
- Update Reader & Writer
- Update Reader & Writer
There was a problem hiding this comment.
See my comments.
I'll note that this incorrect Fressian handler doesn't cause problems in practice because Clara durability is tuned to be done only on "stable working memory states". This means after fire-rules and before and subsequent calls to insert, retract, and/or related.
It also supports serialization of working memory that has never had insert, retract, and/or related.
RuleOrderedActivation, and any other "activation memory" sort of constructs, only exists in this interim state prior to fire-rules.
The change still makes sense (after it is fixed) to not leave broken handlers though.
| @@ -492,11 +492,13 @@ | |||
| (.writeObject w (.-node-id ^RuleOrderedActivation c) true) | |||
| (.writeObject w (.-token ^RuleOrderedActivation c)) | |||
| (.writeObject w (.-activation ^RuleOrderedActivation c)) | |||
| (.writeInt w (.-rule-load-order ^RuleOrderedActivation c)))) | |||
| (.writeInt w (.-rule-load-order ^RuleOrderedActivation c)) | |||
| (.writeBoolean w (.-use-token-identity? ^RuleOrderedActivation c)))) | |||
There was a problem hiding this comment.
Clojure makes fields with metadata :unsynchronized-mutable private by default.
RuleOrderedActivation.use-token-identity? is marked with this metadata, so it is private.
I'd expect 2 things here:
- a reflection warning at compile time for the call
(.-use-token-identity? ^RuleOrderedActivation c)since that private field isn't exposed so the clj compiler cannot emit an appropriate callsite for it. - a runtime exception thrown when this is evaluated due to the field not being found (via reflection due to (1) already happening).
In clara.rules.memory you'll see a protocol IdentityComparable that is used in order to be able to implement at "setter" on RuleOrderedActivation for this private field.
There is no protocol function implemented that gives access to this field however. It may be reasonable to add another function to the IdentityComparable protocol, such as use-token-identity? that will return the value of this field for serialization. This protocol is really only an internal impl detail around this optimization, so that should be fine.
It wouldn't be too difficult to test this handler "round tripping" a RuleOrderedActivation to expose the problem.
There was a problem hiding this comment.
Example of testing it:
(require 'clara.test-fressian)
(in-ns 'clara.test-fressian)
(require '[clara.rules.memory :as mem])
(let [act (mem/->RuleOrderedActivation 1 nil nil 1 false)]
(test-serde act))…yComparable - Update RuleOrderActivation Fressian Handler to appropriately - Add test
|
👍 |
|
👍 |
| @@ -2,6 +2,7 @@ This is a history of changes to clara-rules. | |||
|
|
|||
| ### 0.19.0-SNAPSHOT | |||
| * Remove a warning about `qualified-keyword?` being replaced when using Clojure 1.9. | |||
| * Fix RuleOrderedActivation Fressian Handler. See [issue 394](https://github.com/cerner/clara-rules/issues/394) | |||
There was a problem hiding this comment.
Could we indicate directly in the changelog that the functional impact here is very limited as discussed at #394 (comment) and #395 (review) ? I'd like to avoid unnecessary user concern. Something like "The bug is only exposed if reusing the Clara Fressian handlers outside of Clara's direct APIs"
| @@ -492,11 +492,13 @@ | |||
| (.writeObject w (.-node-id ^RuleOrderedActivation c) true) | |||
| (.writeObject w (.-token ^RuleOrderedActivation c)) | |||
| (.writeObject w (.-activation ^RuleOrderedActivation c)) | |||
| (.writeInt w (.-rule-load-order ^RuleOrderedActivation c)))) | |||
| (.writeInt w (.-rule-load-order ^RuleOrderedActivation c)) | |||
| (.writeBoolean w (mem/use-token-identity? ^RuleOrderedActivation c)))) | |||
There was a problem hiding this comment.
Why do you have the type hint on c here? Type hints are normally used so that Clojure can resolve a Java interop call to a particular function at compile time. With protocol functions the workflow in the end is something like:
- Clojure calls the protocol function like any other Clojure function. The function has a method like invoke(Object arg1 .. Object argN) if you look at the emitted bytecode.
- That function searches for the implementation on the type in question; see here if you're curious.
This will actually be a bit more performant if you invoke the JVM method directly; it will be mangled a bit for the ? I think but it will exist. The reason to use the protocol function instead is if you extend the protocol outside the definition of the type, for example with extend-protocol, but that isn't the case here. If you do this you'll want the type hint. Might be good to do for the sake of the pattern although it doesn't really matter here unless a user reuses the handler in a hotspot. That sort of thing could matter a bit if you were doing it on a type here that is actually used though and it is an easy optimization to make.
There was a problem hiding this comment.
The hint isn't useful. I think it is just left over from when the field was being directly accessed here in the first revision of this PR (that wouldn't work though since it is a private field and that's why this change was made).
I don't think that direct interop with a protocol is going to be important here. The inline caching would likely be just about the same and this isn't a particularly "hot spot" (not even used by durability at this point and not used elsewhere since it was broken before this). Best to stay basic when it can be.
There was a problem hiding this comment.
Not a big deal either way as you say. I'm OK with using the protocol here if we make this change, I mainly just wanted to raise awareness for cases where it might matter.
| @@ -492,11 +492,13 @@ | |||
| (.writeObject w (.-node-id ^RuleOrderedActivation c) true) | |||
There was a problem hiding this comment.
Could we recap the discussion about the non-usage of this handler in a sentence or two of comments inline here?
|
Left a couple comments. My primary concern is https://github.com/cerner/clara-rules/pull/395/files#r194922754 - if we do this I'd like to be clear in our communication with users in the changelog, eventual release announcement, etc. so that we don't cause unnecessary concern when they won't be impacted anyway, which to recap should be any user who is using the public APIs. |
