Re: Reftable locking on Windows (Re: [PATCH 1/6] fixup! reftable: rest of library)

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Dec 02 2020, Han-Wen Nienhuys wrote:

> On Wed, Dec 2, 2020 at 1:32 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>> > Consider processes P1 and P2, and the following sequence of actions
>> >
>> > P1 opens ref DB (ie. opens a set of *.ref files for read)
>> > P2 opens ref DB, executes a transaction. Post-transaction, it compacts
>> > the reftable stack.
>> > P2 exits
>> > P1 exits
>> >
>> > Currently, the compaction in P2 tries to delete the files obviated by
>> > the compaction. On Windows this currently fails, because you can't
>> > delete open files.
>>
>> Indeed. So the design needs to be fixed, if it fails.
>>
>> > There are several options:
>> >
>> > 1) P2 should fail the compaction. This is bad because it will lead to
>> > degraded performance over time, and it's not obvious if you can
>> > anticipate that the deletion doesn't work.
>> > 2) P2 should retry deleting until it succeeds. This is bad, because a
>> > reader can starve writers.
>> > 3) on exit, P1 should check if its *.ref files are still in use, and
>> > delete them. This smells bad, because P1 is a read-only process, yet
>> > it executes writes. Also, do we have good on-exit hooks in Git?
>> > 4) On exit, P1 does nothing. Stale *.ref files are left behind. Some
>> > sort of GC process cleans things up asynchronously.
>> > 5) The ref DB should not keep files open, and should rather open and
>> > close files as needed; this means P1 doesn't keep files open for long,
>> > and P2 can retry safely.
>> >
>> > I think 3) seems the cleanest to me (even though deleting in read
>> > process feels weird), but perhaps we could fallback to 5) on windows
>> > as well.
>>
>> Traditionally, Git would fail gracefully (i.e. with a warning) to delete
>> the stale files, and try again at a later stage (during `git gc --auto`,
>> for example, or after the next compaction step).
>
> So, how does this sound:
>
> * add a void
>
>   set_warn_handler(void(*handler)(char const*))
>
> The handler is called for non-fatal errors (eg. deleting an in-use
> .ref file during compaction), and can provide the warning.
>
> * all .ref files will be prefixed with an 8-byte random string, to
> avoid that new *.ref files collide with unreferenced, stale *.ref
> files.

Just trying to follow along here. Do you mean prefix the file name or
the content of those *.ref files? In any case isn't this synonymous with
proposing moving beyond reftable v1 (to the WIP v2, or a v1.1 with just
this change?). The current spec seems to preclude prefixing either the
file content or filename, but maybe I'm misreading it:

https://googlers.googlesource.com/sop/jgit/+/reftable/Documentation/technical/reftable.md#update-transactions
https://googlers.googlesource.com/sop/jgit/+/reftable/Documentation/technical/reftable.md#header

When the reftable format was initially being discussed I suggested
somewhat flippantly (why not SQLite?). Partially because in trying to
invent what's essentially a portable and concurrently updated database
format we'd be likely to reinvent much of the same wheel.

So not to drag that whole thing up again as a proposal for a format
replacement, but I wonder what SQLite would do in this scenario & others
where desired DB semantics / transactions and FS/portability semantics
clash. AFAIK the reftable has only been battle-tested in production
under JGit so far (presumably Linux-only). Whereas SQLite has been
ported to and widely used on Windows, HP/UX and probably other systems
where Linux-specific assumptions don't apply.

> * in reftable_stack_close(), after closing files, we check if *.ref
> files we were reading have gone out of use. If so, delete those .ref
> files, printing a warning on EACCESS.
>
> * an on-exit handler in git calls reftable_stack_close()
>
> * provide a reftable_stack_gc(), which loads tables.list and then
> deletes all unused *.ref files that are older than tables.list.
>
>> > What errno code does deleting an in-use file on Windows produce?
>>
>> I believe it would be `EACCES`. See
>> https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/unlink-wunlink?view=msvc-160
>> for the documented behavior (I believe that an in-use file is treated the
>> same way as a read-only file in this instance).
>
> your commit message says "to avoid the prompt complaining that a
> `.ref` file could not be deleted on Windows." AFAICT, this prompt is
> coming from windows itself, because the reftable library doesn't issue
> this type of warning. Is there a way to delete a file that just
> returns EACCESS if it fails, without triggering a prompt? Or is the
> prompt part of the "failing gracefully" behavior?
>
> -- 
> Han-Wen Nienhuys - Google Munich
> I work 80%. Don't expect answers from me on Fridays.