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

[Han-Wen Nienhuys <hanwen@google.com>]

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.

* 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.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado