Per

Given a hierarchy of tags with parents:

and methods defined on ::a and ::c, the global version can set a preference for ::b over ::a to resolve the ambiguity when invoked with d:

But the same technique does not work with local hierarchies:

Cause: The code in prefers() calls parents but does not pass the hierarchy to use (so global is always used) - it should be passing the multimethod’s hierarchy instead.

Additionally, the other internal private methods that do preference checking (prefers, isA, dominates) use inconsistent strategies for using the hierarchy state - prefers always uses global hierarchy, isA derefs the multimethod hierarchy but ignores the cachedHierarchy, and dominates calls both of the others.

There are two users of these methods - preferMethod (which sets a preference and does all of this under write lock) uses these to make validation checks - in this case it makes the most sense to deref the hierarchy, use it for validation, update the preference table, and then reset the cache. findAndCacheBestMethod uses all of these methods under an optimistic read lock to choose best methods, and then has a strategy to verify the multimethod state has not drifted before caching the result. In the latter case, we really want to use the cachedHierarchy throughout to get a consistent view during the preference decision, but right now we can see live hierarchy updates in isA and the wrong hierarchy in prefers.

Proposed: Change all of prefers, isa, and dominates to explicitly take the hierarchy to use - these methods then all become stateless and easier to reason about. preferMethod should deref the hierarchy to get newest state (ignore the cachedHierarchy), update the preference, and then reset the cache. There is still a race here between the validation check and the preference table update where the hierarchy could be updated in another thread. Without using a much different strategy, I don’t think we can atomically update the preference table (under the multimethod write lock) and also exclude a concurrent change to the hierarchy var, certainly this race and other similar ones have always existed.

findAndCacheBestMethod already gets the cachedHierarchy and should just explicitly use it in the calls to these methods. The final state check at the end may cause a recompute in the case of drift.

Patch: clj-2234-4.patch - uses the tests from -3 but new code for the fix.

Screened By: fogus