[v2] read-cache: free threaded memory pool

Commit Message

Derrick Stolee Oct. 1, 2024, 5:37 p.m. UTC

From: Derrick Stolee <[e-mail]>

In load_cache_entries_threaded(), each thread allocates its own memory
pool. This pool needs to be cleaned up while closing the threads down,
or it will be leaked.

This ce_mem_pool pointer could theoretically be converted to an inline
copy of the struct, but the use of a pointer helps with existing lazy-
initialization logic. Adjusting that behavior only to avoid this pointer
would be a much bigger change.

Signed-off-by: Derrick Stolee <[e-mail]>
---
    read-cache: two small leak fixes
    
    This v2 removes the duplicate patch and updates the commit message.
    
    Thanks, -Stolee

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1801%2Fderrickstolee%2Fleaks-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1801/derrickstolee/leaks-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1801

Range-diff vs v1:

 1:  9a45b15ea4b ! 1:  220a098c93c read-cache: free threaded memory pool
     @@ Metadata
       ## Commit message ##
          read-cache: free threaded memory pool
      
     -    In load_cache_entries_threaded(), each thread is allocated its own
     -    memory pool. This pool needs to be cleaned up while closing the threads
     -    down, or it will be leaked.
     +    In load_cache_entries_threaded(), each thread allocates its own memory
     +    pool. This pool needs to be cleaned up while closing the threads down,
     +    or it will be leaked.
     +
     +    This ce_mem_pool pointer could theoretically be converted to an inline
     +    copy of the struct, but the use of a pointer helps with existing lazy-
     +    initialization logic. Adjusting that behavior only to avoid this pointer
     +    would be a much bigger change.
      
          Signed-off-by: Derrick Stolee <[e-mail]>
      
 2:  b6fe5b3ef7e < -:  ----------- read-cache: free hash context in do_write_index()


 read-cache.c | 1 +
 1 file changed, 1 insertion(+)


base-commit: 6258f68c3c1092c901337895c864073dcdea9213

Comments

Patrick Steinhardt Oct. 2, 2024, 10:49 a.m. UTC | #1

On Tue, Oct 01, 2024 at 05:37:44PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <[e-mail]>
> 
> In load_cache_entries_threaded(), each thread allocates its own memory
> pool. This pool needs to be cleaned up while closing the threads down,
> or it will be leaked.
> 
> This ce_mem_pool pointer could theoretically be converted to an inline
> copy of the struct, but the use of a pointer helps with existing lazy-
> initialization logic. Adjusting that behavior only to avoid this pointer
> would be a much bigger change.

Thanks, this looks good to me.

Patrick

Patch

diff --git a/read-cache.c b/read-cache.c
index 764fdfec465..3c078afadbc 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2188,6 +2188,7 @@  static unsigned long load_cache_entries_threaded(struct index_state *istate, con
 		if (err)
 			die(_("unable to join load_cache_entries thread: %s"), strerror(err));
 		mem_pool_combine(istate->ce_mem_pool, p->ce_mem_pool);
+		free(p->ce_mem_pool);
 		consumed += p->consumed;
 	}