[PATCH STABLE] localrepo: clear filecache correctly on destroyed()

[Idan Kamara]

On Tue, Nov 20, 2012 at 12:12 AM, Hao Lian <hao at fogcreek.com> wrote:
> # HG changeset patch
> # User Hao Lian <hao at fogcreek.com>
> # Date 1353360901 18000
> # Branch stable
> # Node ID 0d053ca02a3d94e56634091690a9a58c8d833329
> # Parent  35ba170c0f82dba18f0207ef4bd93216e6de8bbf
> localrepo: clear filecache correctly on destroyed()
>
> Assume R is a localrepo and P is a property of R decorated with storecache.
> Prior to 9f94358, invalidate() enforced the invariant that P is a member of
> R.__dict__ if and only if it were also mirrored in R's filecache.  Now, however,

That's correct, except the behavior prior to 9f94358 was a workaround
that disabled the whole thing:

http://mercurial.markmail.org/thread/34cv3eadjods7ca6

For reference, this piece of code went through these changes:

http://selenic.com/repo/hg/rev/ce0ad184f489
http://selenic.com/repo/hg/rev/9a99224a6409
http://selenic.com/repo/hg/rev/357e6bcfb619
http://selenic.com/repo/hg/rev/9f94358f9f93

> P may be deleted from filecache without the corresponding object being deleted
> in R.__dict__.  This allows the following bug to occur:

Ok, that's probably not what we want (although the other way is
perfectly ok!), and I see that it can happen when destroyed is
called without invalidate being called before.

>
> * P is deleted from R's filecache.
> * The underlying file that P corresponds to is updated.
> * Someone attempts to access P.
> * Because the stale P is in __dict__, storecache returns stale P.
>
> Prior to 9f94358, P would have been deleted from R's both __dict__ and
> filecache, never just one of them.
>
> This exact bug occurs during hg convert, which rolls back empty revisions when
> converting Subversion repositories.  When that happens, _rollback() calls
> destroyed(), which deletes the changelog object from filecache without deleting
> it from __dict__.  As a result, the changelog becomes stale and len(changelog)
> is one larger than the actual length of the changelog; as the conversion
> progresses, hg convert (sometimes) errors out during an integrity check on the
> changelog.
>
> The fix is to simply undo the changes of 9f94358.  However, we still want
> destroyed() to clear the filecache, which was the intent of that changeset.

Yes, but clearing filecache during invalidate isn't the solution.

Here's a rundown on the basics of it using your definitions. Assume P
wasn't accessed yet and let's call F the underlying file P is caching:

1) a call to R.P is made
2) we check if _filecache[P] exists

it doesn't:
3) call P and store the result along with stat info of F in _filecache[P]

it does:
4) get a fresh stat of F and compare it to the old one in _filecache[P]
5) if they're identical then F hasn't changed since the last time it was
    read, just return the old object
6) otherwise call P and store the result with stat info in _filecache[P]

So if we clear _filecache on invalidate, we'll always hit 3) after
invalidating which is exactly what this whole thing was designed
to solve.

Perhaps we want to pass an argument to invalidate that tells
it whether to clear the filecache, and use it during _rollback.

> That is accomplished by calling invalidate(), which (now) enforces the
> invariant.  Since invalidate() calls invalidatecaches() anyway, we can simply
> replace the call to invalidatecaches() with invalidate().

This means that callers of destroyed (strip) who didn't go through
_rollback will now call invalidate rather than invalidatecaches. Can't
tell if we want this.

I'd like to better understand the root of the bug and I can't seem to
reproduce it without going through convert. Were you able to create a
self contained test?