Re: [PATCH 1/6] fixup! reftable: rest of library
- From
Han-Wen Nienhuys <hanwen@google.com>
- Date
- Dec 1, 2020, 14:32 UTC
- Message-ID
- <CAFQ2z_N=sZuYorZxNLn-OoWjb6y=1zrc7sCPuKzZHQODi08s+Q@mail.gmail.com>
- In-Reply-To
- <878bffcdfe5ca7657f839de8f7993d9098726636.1606545878.git.gitgitgadget@gmail.com>
On Sat, Nov 28, 2020 at 7:44 AM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:
Show 18 quoted lines
> > 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?
Show 103 quoted lines
> 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