Patrick Steinhardt <ps@xxxxxx> writes:
> The lifetime of a table iterator may surive the lifetime of a reader
s/surive/survive
> when the stack gets reloaded. Keep the reader from being released by
> increasing its refcount while the iterator is still being used.
>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> reftable/reader.c | 2 ++
> reftable/stack_test.c | 49 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 51 insertions(+)
>
> diff --git a/reftable/reader.c b/reftable/reader.c
> index 64a0953e68..f877099087 100644
> --- a/reftable/reader.c
> +++ b/reftable/reader.c
> @@ -175,6 +175,7 @@ static int table_iter_init(struct table_iter *ti, struct reftable_reader *r)
> {
> struct block_iter bi = BLOCK_ITER_INIT;
> memset(ti, 0, sizeof(*ti));
> + reftable_reader_incref(r);
> ti->r = r;
> ti->bi = bi;
> return 0;
> @@ -262,6 +263,7 @@ static void table_iter_close(struct table_iter *ti)
> {
> table_iter_block_done(ti);
> block_iter_close(&ti->bi);
> + reftable_reader_decref(ti->r);
> }
>
> static int table_iter_next_block(struct table_iter *ti)
> diff --git a/reftable/stack_test.c b/reftable/stack_test.c
> index bc3bf77274..91e716dc0a 100644
> --- a/reftable/stack_test.c
> +++ b/reftable/stack_test.c
> @@ -1076,6 +1076,54 @@ static void test_reftable_stack_compaction_concurrent_clean(void)
> clear_dir(dir);
> }
>
> +static void test_reftable_stack_read_across_reload(void)
> +{
> + struct reftable_write_options opts = { 0 };
> + struct reftable_stack *st1 = NULL, *st2 = NULL;
> + struct reftable_ref_record rec = { 0 };
> + struct reftable_iterator it = { 0 };
> + char *dir = get_tmp_dir(__LINE__);
> + int err;
> +
> + /* Create a first stack and set up an iterator for it. */
> + err = reftable_new_stack(&st1, dir, &opts);
> + EXPECT_ERR(err);
> + write_n_ref_tables(st1, 2);
> + EXPECT(st1->merged->readers_len == 2);
> + reftable_stack_init_ref_iterator(st1, &it);
> + err = reftable_iterator_seek_ref(&it, "");
> + EXPECT_ERR(err);
> +
> + /* Set up a second stack for the same directory and compact it. */
> + err = reftable_new_stack(&st2, dir, &opts);
> + EXPECT_ERR(err);
> + EXPECT(st2->merged->readers_len == 2);
> + err = reftable_stack_compact_all(st2, NULL);
Shouldn't we verify that `EXPECT(st2->merged->readers_len == 1);` here?
> + EXPECT_ERR(err);
> +
> + /*
> + * Verify that we can continue to use the old iterator even after we
> + * have reloaded its stack.
> + */
> + err = reftable_stack_reload(st1);
> + EXPECT_ERR(err);
> + EXPECT(st2->merged->readers_len == 1);
Oh we do it here, I would've expected it above the `st1` reload. And
probably expected `EXPECT(st1->merged->readers_len == 2);` here to
confirm that we still have the readers.
> + err = reftable_iterator_next_ref(&it, &rec);
> + EXPECT_ERR(err);
> + EXPECT(!strcmp(rec.refname, "refs/heads/branch-0000"));
> + err = reftable_iterator_next_ref(&it, &rec);
> + EXPECT_ERR(err);
> + EXPECT(!strcmp(rec.refname, "refs/heads/branch-0001"));
> + err = reftable_iterator_next_ref(&it, &rec);
> + EXPECT(err > 0);
> +
> + reftable_ref_record_release(&rec);
> + reftable_iterator_destroy(&it);
> + reftable_stack_destroy(st1);
> + reftable_stack_destroy(st2);
> + clear_dir(dir);
> +}
> +
> int stack_test_main(int argc, const char *argv[])
> {
> RUN_TEST(test_empty_add);
> @@ -1098,6 +1146,7 @@ int stack_test_main(int argc, const char *argv[])
> RUN_TEST(test_reftable_stack_auto_compaction_fails_gracefully);
> RUN_TEST(test_reftable_stack_update_index_check);
> RUN_TEST(test_reftable_stack_uptodate);
> + RUN_TEST(test_reftable_stack_read_across_reload);
> RUN_TEST(test_suggest_compaction_segment);
> RUN_TEST(test_suggest_compaction_segment_nothing);
> return 0;
> --
> 2.46.0.164.g477ce5ccd6.dirty
Attachment:
signature.asc
Description: PGP signature
