Make LocalCache not use synchronized to detect recursive loads.

[copybara-service]

Make LocalCache not use synchronized to detect recursive loads.

Fixes #6851
Fixes #6845

RELNOTES=n/a

[ben-manes]

To fix compute bugs, I added ComputingValueReference extends LoadingValueReference where isLoading is false (42bf4f4). You'd still want recursive detection, so I think checking by the type instead of the flag is better here.

[cpovirk]

Whoa, this is complicated! Thanks.

Hmm, does the right thing even currently happen if someone starts a load when compute is already running? If ComputingValueReference returns false from isLoading, then would we call valueReference.get()... and then either get back null (if there was no previous value) and say an entry was COLLECTED (which seems iffy) or get back the previous value (which might be fine but which I haven't thought about)? We probably never get to waitForLoadingValue and the deadlock detection at all? (If we did, we'd trigger the AssertionError above!)

Maybe there's sketchy stuff going on but this PR at least preserves existing behavior?

[cpovirk]

#5439? #3719? I am going to try not to get sucked much deeper into this. Use Caffeine, everyone! (And hopefully we can still make this "simple" virtual-thread fix :))

[ben-manes]

yeah, there are lots of little gotchas and I may have introduced some in my fix attempts too. I'd only suggest modifying that comment and certainly not expand the scope. Even Doug had challenges with recursive computes and there are so many interactions that I couldn't possibly keep them all in my head (hence 10M test executions per build in hopes to catch regressions).

[cpovirk]

Sounds good. I have tweaked the comment to avoid implying that instanceof LoadingValueReference implies isLoading().

[ben-manes]

Hmm, does the right thing even currently happen if someone starts a load when compute is already running?

oh, to answer your question this cannot happen because the compute methods are implemented very coarsely. They hold the segment lock during the computation which blocks all other writes to that segment, meaning a loader would wait to obtain the lock and by then the computing value reference would be gone.

The alternative would be to use retry loops on all write paths by taking the segment lock, if loading then waiting on the entry to complete outside of the segment lock, or else perform its entry action accordingly. That would have been far more invasive and may not have maintained linearizability, so I believe simpler approach was used just to avoid accidents as the default method version is non-atomic.

ConcurrentHashMap uses fine-grained segments that are dynamically added, so it similarly locks rather than using retry loops. Caffeine delegates loading to computeIfAbsent as merely an alias, where the only negative effect is long-running computes could impact siblings. Thus, AsyncCache mitigates that when an issue since it sacrifices linearization of the loading value (only of the future mapping) so that the map operations are fast, and the synchronous view uses the retry loops approach as best-effort (not linearizable as it fails Lincheck).

[cpovirk]

Oh, good. I'm glad that we at least do not have an additional bug on top of the one you already reported... :)

[cortlepp]

Hey, can you give me an update regarding the current status of this PR?

[cpovirk]

Hi, sorry.

The testing results are good. My reviewer for the internal change had some other things come up, so I'll reassign the review to someone else.

While we're here: How important is it to you for us to publish a release that contains this change? I'm hoping to publish one soonish no matter what, but it would be useful to know specifically about this change.

[cortlepp]

No problem,

Ok 👍 .

I don't need one right away, if that's what you mean. I have built a version of guava from this branch locally which I am already using for some tests, and that's totally fine for now. It would of course be cool if it were already included in the next release, but it is not a necessity for me.