Re: [PATCH 1/6] fixup! reftable: rest of library

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

On Sat, Nov 28, 2020 at 7:44 AM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Close the file descriptors to obsolete files before trying to delete or
> rename them. This is actually required on Windows.
>
> Note: this patch is just a band-aid to get the tests pass on Windows.
> The fact that it is needed raises concerns about the overall resource
> handling: are file descriptors closed properly whenever appropriate, or
> are they closed much later (which can lead to rename() problems on
> Windows, and risks running into ulimits)?
>
> Also, a `reftable_stack_destroy()` call had to be moved in
> `test_reftable_stack_uptodate()` to avoid the prompt complaining that a
> `.ref` file could not be deleted on Windows. This raises the question
> whether the code does the right thing when two concurrent processes want
> to access the reftable, and one wants to compact it. At the moment, it
> does not appear to fail gracefully.

Thanks for the report; I have to look more closely at these fixes; I
fear they might be incorrect.

The reftable spec doesn't treat this case in depth, and I think it was
rather written for Unix-like semantics. In the Unix flavor, a process
that wants to read can keep file descriptors open to keep reading from
the ref DB at a consistent snapshot.

What is the approach that the rest of Git on Windows takes in these
circumstances?

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.

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.

Thoughts?

What errno code does deleting an in-use file on Windows produce?


> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  reftable/stack.c      | 37 ++++++++++++++++++++++++++++---------
>  reftable/stack_test.c |  2 +-
>  2 files changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/reftable/stack.c b/reftable/stack.c
> index 1d632937d7..02c6a370ba 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -212,7 +212,6 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,
>                 goto done;
>
>         new_tables = NULL;
> -       st->readers_len = new_readers_len;
>         if (st->merged != NULL) {
>                 merged_table_release(st->merged);
>                 reftable_merged_table_free(st->merged);
> @@ -220,6 +219,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,
>         if (st->readers != NULL) {
>                 reftable_free(st->readers);
>         }
> +       st->readers_len = new_readers_len;
>         st->readers = new_readers;
>         new_readers = NULL;
>         new_readers_len = 0;
> @@ -939,14 +939,6 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,
>         strbuf_addstr(&new_table_path, "/");
>         strbuf_addbuf(&new_table_path, &new_table_name);
>
> -       if (!is_empty_table) {
> -               err = rename(temp_tab_file_name.buf, new_table_path.buf);
> -               if (err < 0) {
> -                       err = REFTABLE_IO_ERROR;
> -                       goto done;
> -               }
> -       }
> -
>         for (i = 0; i < first; i++) {
>                 strbuf_addstr(&ref_list_contents, st->readers[i]->name);
>                 strbuf_addstr(&ref_list_contents, "\n");
> @@ -960,6 +952,32 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,
>                 strbuf_addstr(&ref_list_contents, "\n");
>         }
>
> +       /*
> +        * Now release the merged tables and readers
> +        */
> +       if (st->merged != NULL) {
> +               reftable_merged_table_free(st->merged);
> +               st->merged = NULL;
> +       }
> +
> +       if (st->readers != NULL) {
> +               int i = 0;
> +               for (i = 0; i < st->readers_len; i++) {
> +                       reader_close(st->readers[i]);
> +                       reftable_reader_free(st->readers[i]);
> +               }
> +               st->readers_len = 0;
> +               FREE_AND_NULL(st->readers);
> +       }
> +
> +       if (!is_empty_table) {
> +               err = rename(temp_tab_file_name.buf, new_table_path.buf);
> +               if (err < 0) {
> +                       err = REFTABLE_IO_ERROR;
> +                       goto done;
> +               }
> +       }
> +
>         err = write(lock_file_fd, ref_list_contents.buf, ref_list_contents.len);
>         if (err < 0) {
>                 err = REFTABLE_IO_ERROR;
> @@ -1242,6 +1260,7 @@ static int stack_check_addition(struct reftable_stack *st,
>
>         free(refs);
>         reftable_iterator_destroy(&it);
> +       reader_close(rd);
>         reftable_reader_free(rd);
>         return err;
>  }
> diff --git a/reftable/stack_test.c b/reftable/stack_test.c
> index 11d3d30799..c35abd7301 100644
> --- a/reftable/stack_test.c
> +++ b/reftable/stack_test.c
> @@ -159,12 +159,12 @@ static void test_reftable_stack_uptodate(void)
>         err = reftable_stack_add(st2, &write_test_ref, &ref2);
>         EXPECT(err == REFTABLE_LOCK_ERROR);
>
> +       reftable_stack_destroy(st1);
>         err = reftable_stack_reload(st2);
>         EXPECT_ERR(err);
>
>         err = reftable_stack_add(st2, &write_test_ref, &ref2);
>         EXPECT_ERR(err);
> -       reftable_stack_destroy(st1);
>         reftable_stack_destroy(st2);
>         clear_dir(dir);
>  }
> --
> gitgitgadget
>


-- 
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