Re: [PATCH 1/2] read-cache: free threaded memory pool

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 30, 2024, 12:32 UTC
Message-ID: <ZvqaTqegJxow1x-b@pks.im>
In-Reply-To: <9a45b15ea4b9864cd3cff066ecd9281c4539d5f7.1727696424.git.gitgitgadget@gmail.com>

On Mon, Sep 30, 2024 at 11:40:23AM +0000, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <stolee@gmail.com>
> 
> In load_cache_entries_threaded(), each thread is allocated its own

s/allocated/allocating/

Show 19 quoted lines

> memory pool. This pool needs to be cleaned up while closing the threads
> down, or it will be leaked.
> 
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
>  read-cache.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> 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;
>  	}

Okay. We move over the contents of the pool, but forgot to free the pool itself. As far as I can see the pool is always allocated and only used in two functions, both of which assume that it is allocated. So I wonder why it is allocated in the first place instead of making it a direct member of `struct load_cache_entries_thread_data`.

Patrick