Fix RuleOrderedActivation Fressian Handler
Opened this issue · 6 comments
The Fressian Handler for RuleOrderedActiviation listed here has an incorrect WriteHandler and ReadHandler. Looking at the RuleOrderedActivation deftype it seems as though use-token-identity? is missing.
The write handler should be updated with writeBoolean to correctly write out a RuleOrderedActivation and the read handler should be updated with an extra readObject.
c.c. @mrrodriguez & @WilliamParker
After the changes for #268 I don't think RuleOrderedActivation will be exposed to the user since it will only be created after a fire-rules call and they will all be performed before fire-rules returns. Insert or retract calls could do it before but now they're just stored until fire-rules is called. I'll have to look at if listeners could expose this later/not on mobile.
@WilliamParker I mentioned this in #395 (review)
However, if we are going to have the Fressian handler defined, it does make sense to not have a broken one (to avoid future surprises perhaps).
So I'm fine with the PR and will +1 it.
Yeah, agreed that if we are going to have the handler it is better that it be correct.
That said, looking over session serialization again I don't see any attempt to serialize listeners (looking particularly at indexed-session-memory-state) so I don't see any scenario where it would be possible to call the RuleOrderedActivation handler using Clara's direct APIs. Given that, my inclination would be to just remove the handler entirely in the interest of having less code to maintain. Serialization of sessions with pending operations from insert or retract looks like it would probably be broken now anyway since I don't see any attempt to serialize those operations, but I don't see any reason why it couldn't be supported.
However, I do see some utility in having the hander around for users to incorporate in serialization of listeners. Listeners do take activations as for add-activations! and remove-activations! so a user having a legitimate need to serialize the type is plausible. And if we have it then it should be tested, which as @mrrodriguez says needs to be done with direct SerDe calls. So if we want to have the handler still that seems OK.
Given that, my inclination would be to just remove the handler entirely in the interest of having less code to maintain.
Removing it would be fine as well. I don't have particularly strong feelings either way. It is a pretty small thing and perhaps it could at some point be useful if we did try to serialize interim states of sessions.
Regarding listener stuff, yeah, they were left out of serialization. They can be attached during serialization time if I recall correctly. Not really a relevant topic to this change though.
"and perhaps it could at some point be useful if we did try to serialize interim states of sessions." =>
I don't think that would happen even in that case unless we got rid of the caching of pending operations. See here in insert and here in retract; note the lack of any interaction with the Rete network.
My point with listeners was that someone might reuse the stock Clara Fressian handlers in their own listener serialization implementation. On second thought though I don't think I was thinking clearly the first time, it is the engine's Activation record, not the memory's RuleOrderedActivation that is exposed to listeners.