`StoreRequest.cached(1, refresh = false)` invoke `fetcher` when cache available? about store

[digitalbuddha]

I would expect to only receive a cached value if one exists otherwise hit the fetcher. Open for other's thoughts

[yigit]

yea seems like a bug when there is no source of truth.
have the failing test, will work on a fix.

yigit@3c2353e

[yigit]

actually we had a prior implementation that faked source of truth if not provided. I cannot remember why i removed it but this is kind of related to that (to make cache step up its game).
I'll be working on this but does not seem like a super trivial change, of course definitely doable :) just might be a bigger change than expected 🤷‍♂️

[ychescale9]

Currently without specifying a persister (source of truth), the fetcher will always be run regardless of whether refresh Is true, although you’ll get the cached value as the first emission.

I also find this behavior a bit confusing. @digitalbuddha @yigit when sourceOfTruth is null, should the cache effectively become the source of truth (in the example above it will just return the cached value without hitting the network)?

[yigit]

Yea seems reasonable. let me dig into code to understand what is going on and why it doesn't work that way.

[yigit]

Related CL where the logic to make previous fetchers receive new values is implemented:

3e84b6b

[yigit]

I have a bit of a dirty implementation that create a source of truth on top of cache. It is not good enough to be merged yet because I don't feel comfortable it is well covered. I'll keep working on it.

The good thing about that solution is that it does not create a third code path to handle having no source of truth but cache, on the other hand, there has been assumptions that source of truth is disk so I don't feel super comfortable right now that it has full coverage and it might even be cleaner to consider it a third case

This passes all tests but i'm afraid some tests might be passing because they don't collect enough many items even if there could be more items.
https://github.com/yigit/Core/tree/cached-bug

[eyalgu]

Here's an alternative approach for solving this bug without a fake source of truth:
I added the concept of "piggyback only channel" to channel manager. if you add a piggyback only channel it will not kick off a fetch but will receive downstream piggybacks (if piggybacks is enabled). This works pretty easily with the current design of channel manager (by adding an entry that does not require a dispatch).

Still need to do due diligance to make sure this doesn't mess up the ack logic, but wanted to get your thoughts about this approach before going any farther.

@yigit's test pass with this fix

5f058fd

[eyalgu]

Thoughts on cached source of Truth inline in @yigit's bench e1e52d4#r36863862

[yigit]

hmm i'm a bit torn.
On one end, this is less logical change in the code which is nice.
On the other hand, I've considered adding that piggybacking a bit hacky (3e84b6b) at the time and was thinking of changing the fix for this to have a source of truth all the time (hence not rely on piggybacking). Even though it is more code, it will have simpler logic in RealStore.

Wdyt ? In either case, I think either your piggyback extension or an always available source of truth is better solution than the one I currently have which adds source of truth only if cache is enabled.

[eyalgu]

I have less context than you on the history of multicaster, but I actually don't think piggybackOnly downstream is hacky.

My reasoning is that that API already allows the user to control piggybacking, which controls whether a channel that has already completed should stick around in piggyback mode. The reason behind this are perhaps specific to Store, but non-the-less it exists.
Given that, it seems reasonable that you could also have the option of adding a channel straight to piggyback mode.

I personally dislike the idea of having to multiplex twice inside store. That seems pretty fragile and may have more perf/memory usage implication under load.

[eyalgu]

Let me put up my code as a PR. Maybe it'll help decide on the concept of a piggyback only downstream

[yigit]

From that CL:

It is a bit questionable behavior for multiplexer but it is already very
specific to our use case so I thought it might be fine to do it there

:)
At the time, i found this as an easy solution for the streaming case support. If we go w/ source of truth all the time solution, it probably makes sense to remove that feature or not use it.

If we have a source of truth all the time, it will actually simplify the logic in RealStore as it won't have two paths anymore. That being said, you are right that it will be more code.
I'm not worried about it being fragile as any problem in that multiple source merging logic would already be a problem in the database + fetcher case. In fact, that is why i think it is simpler as it will be one code path.

In either case, I don't feel strongly about either solution so if you strongly prefer the piggyback extension, i'm fine w/ that.

[eyalgu]

#75

[eyalgu]

Yeah I can see us removing piggyback behavior from the multiplexer altogether.
In that case we I think it would be better to introduce a "network sourceOfTruth" when a disk source of truth doesn't exist and let the cache act the same way in both cases.

As you say though, that's a bigger change, but I think we can still move forward with #75 and make a decision about removing piggybacking form the multicaster separately. If we do that we will remove both the existing piggyback support and it's extension from #75

Does that sound reasonable?

[yigit]

I've rebased and uploaded my current fix as well, just for reference:
#76

It does not yet create a source of truth for the fetcher case.

[yigit]

right now i think it is better to go w/ @eyalgu 's PR because my implementation adds a weird cycle where data that goes into cache might come back from cache.
I don't like the complication in Multicaster but it has the least amount of logical change compared to #76 .
@eyalgu , can you copy the tests i have in my CL (added a few more today). Just to make sure it covers them as well.