Always update key dependencies of dirty context cache entries

[frenchy64]

Note: This pull request depends on #35, which changes cljfx.context/invalidate-cache.

Problem

Direct dependencies of subscribed functions are not always synchronized with parent.

In particular, if both

1. A dependency's key-deps changed and
2. the dependency's return value remains the same

then the parent does not inherit changed key-deps.

This is demonstrated in test/cljfx/context_test/context_dirty_entry_updates_key_deps.clj in this pull request.

Here's how it should work:

However, it's broken in several ways, some fixed by #35.

With #35, the sum is not updated until a new button is added

This is because the view's cache entry is only updated when a new button is added, since it only depends on #{::ids}. This is the bug: it should depend on #{::ids ::clicked}.

The sum-buttons function first depends on #{::ids}, then when a button is added, sum-buttons now depends on #{::ids ::clicked}. However, since the sum is still 0, view does not evict its context cache entry (due to the logic in sub-from-dirty, and thus only depends on #{::ids}.

Here is the same app, but without the commits in #35

Many clicks are lost and sum is not also not updated.

Solution

The solution is to update the cache entry of the parent with the new key dependencies (if any) in the case where a dirty cache entry can be converted into a clean one.

A private helper function cljfx.context/register-cache-entry is factored out to share logic between sub and sub-from-dirty.

A minimal test is included: cljfx.context-test/dirty-cache-entry-always-updates-its-key-deps.

A minor enhancement included in this PR is that the subs on dependencies in sub-from-dirty are added to the cache. I added a unit test to ensure this:

        _ (facts
            "[parent-child] recalculated exactly once to verify that [template] cache entry should be evicted"
            @*parent-child-counter => 3)

Without this, @*parent-child-counter becomes 4, because the "miss" case of sub-from-dirty recalculates dependencies even though sub-from-dirty just recalculated them.

To avoid infinite loops between sub-from-dirty and register-cache-entry, *processing-dirty* is the currently processed dirty cache entries.

In the process of trying to test for infinite loops, I came across #36. There is no unit test for infinite loops pending that discussion, but I presume a similar error will be triggered.

Before/after:

[vlaaad]

I couldn't find time to look at it properly, so I'll do it after vacation (at the end of next week, probably)

[frenchy64]

Thanks @vlaaad. I rebased onto master. I also realized I added an extra example cljfx.context-test.context-sub-sometimes that I didn't mention that displayed similar weirdness to the one mentioned above. I'll just leave that in.

[vlaaad]

I think I understand what's the problem (need to refresh key deps of dirty sub), but some other changes I don't understand (*processing-dirty*). Can you please clarify why that's needed?

Anyway, many thanks for your PR, hope to get it merged soon!

[vlaaad]

Are you sure we need to keep key deps of dirty sub? it may introduce unnecessary dependency in a scenario where we had one key dep, and then changed it for another. I tried replacing this line with #{} and it seems to work...

[vlaaad]

Are you sure we need *processing-dirty*? I tried replacing this (if (*processing-dirty sub-id) ...) with just (sub-from-dirty ...) and it seems to work. Can you make a test case where it's needed?

[vlaaad]

I'm not 100% sure, but this bit:

(when *key-deps
  (swap! *key-deps set/union (::key-deps entry)))

might be an important part of register-cache-entry...

Uhh, it's so complicated and intertwined, I fear I created a monster when I did context stuff...

[vlaaad]

I was not sure whether some changes will lead to subtle issues because of how messy context is, so I kept your tests, but made key deps refresh with minimum changes to context code.

Many thanks for discovering an issue with context and coming up with solution!

[frenchy64]

@vlaaad thanks! The reason I had *processing-dirty* is because I wasn't sure if an infinite loop could be introduced between dependencies. IIRC since this made sub and sub-from-dirty mutually recursive, I wanted a safeguard to prevent that.

That's what I was referring to in #36, and it doesn't seem like that's a possible scenario.