| Message ID | 026820562882afb31d7224c90722e09bef835340.1724080006.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | reftable: fix reload with active iterators | expand |
Patrick Steinhardt <ps@pks.im> 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@pks.im> > --- > 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
On Fri, Aug 23, 2024 at 03:21:27AM -0700, karthik nayak wrote: > Patrick Steinhardt <ps@pks.im> writes: > > 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? Yes, we should. > > + 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. And yes, this was a typo, it should've been `st1` indeed. Good eyes, thanks! Patrick
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); + 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); + 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;
The lifetime of a table iterator may surive the lifetime of a reader 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@pks.im> --- reftable/reader.c | 2 ++ reftable/stack_test.c | 49 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+)