git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 08/10] reftable/reader: keep readers alive during iteration

From
karthik nayak <karthik.188@gmail.com>
Date
Aug 23, 2024, 10:21 UTC
Message-ID
<CAOLa=ZQarx37De=xmD73D3dPzxp0j8pfb-TBc4u6nKkJn_TT1g@mail.gmail.com>
In-Reply-To
<026820562882afb31d7224c90722e09bef835340.1724080006.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> The lifetime of a table iterator may surive the lifetime of a reader
s/surive/survive
Show 60 quoted lines
> 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?
Show 9 quoted lines
> +	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.

Show 29 quoted lines
> +	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
Previous: Patrick SteinhardtNext: Patrick Steinhardt
Message 17 of 38 in “reftable: fix reload with active iterators”
  1. 00/10 reftable: fix reload with active iteratorsPatrick Steinhardt, Aug 19, 2024
  2. 01/10 reftable/blocksource: drop malloc block sourcePatrick Steinhardt, Aug 19, 2024
  3. 02/10 reftable/stack: inline `stack_compact_range_stats()`Patrick Steinhardt, Aug 19, 2024
  4. karthik nayakAug 22, 2024
  5. 03/10 reftable/reader: rename `reftable_new_reader()`Patrick Steinhardt, Aug 19, 2024
  6. Justin ToblerAug 22, 2024
  7. Junio C HamanoAug 22, 2024
  8. Patrick SteinhardtAug 23, 2024
  9. 04/10 reftable/reader: inline `init_reader()`Patrick Steinhardt, Aug 19, 2024
  10. 05/10 reftable/reader: inline `reader_close()`Patrick Steinhardt, Aug 19, 2024
  11. 06/10 reftable/stack: fix broken refnames in `write_n_ref_tables()`Patrick Steinhardt, Aug 19, 2024
  12. 07/10 reftable/reader: introduce refcountingPatrick Steinhardt, Aug 19, 2024
  13. karthik nayakAug 22, 2024
  14. Patrick SteinhardtAug 22, 2024
  15. karthik nayakAug 23, 2024
  16. 08/10 reftable/reader: keep readers alive during iterationPatrick Steinhardt, Aug 19, 2024
  17. karthik nayakAug 23, 2024
  18. Patrick SteinhardtAug 23, 2024
  19. 09/10 reftable/stack: reorder swapping in the reloaded stack contentsPatrick Steinhardt, Aug 19, 2024
  20. 10/10 reftable/stack: fix segfault when reload with reused readers failsPatrick Steinhardt, Aug 19, 2024
  21. karthik nayakAug 22, 2024
  22. Jeff KingAug 22, 2024
  23. Patrick SteinhardtAug 22, 2024
  24. Junio C HamanoAug 22, 2024
  25. Patrick SteinhardtAug 23, 2024
  26. Junio C HamanoAug 22, 2024
  27. 00/10 reftable: fix reload with active iteratorsPatrick Steinhardt, Aug 23, 2024
  28. 01/10 reftable/blocksource: drop malloc block sourcePatrick Steinhardt, Aug 23, 2024
  29. 02/10 reftable/stack: inline `stack_compact_range_stats()`Patrick Steinhardt, Aug 23, 2024
  30. 03/10 reftable/reader: rename `reftable_new_reader()`Patrick Steinhardt, Aug 23, 2024
  31. 04/10 reftable/reader: inline `init_reader()`Patrick Steinhardt, Aug 23, 2024
  32. 05/10 reftable/reader: inline `reader_close()`Patrick Steinhardt, Aug 23, 2024
  33. 06/10 reftable/stack: fix broken refnames in `write_n_ref_tables()`Patrick Steinhardt, Aug 23, 2024
  34. 07/10 reftable/reader: introduce refcountingPatrick Steinhardt, Aug 23, 2024
  35. 08/10 reftable/reader: keep readers alive during iterationPatrick Steinhardt, Aug 23, 2024
  36. 09/10 reftable/stack: reorder swapping in the reloaded stack contentsPatrick Steinhardt, Aug 23, 2024
  37. 10/10 reftable/stack: fix segfault when reload with reused readers failsPatrick Steinhardt, Aug 23, 2024
  38. Junio C HamanoAug 23, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.