Skip to content

fix: a couple of memory leaks - #273

Merged
imor merged 1 commit into
masterfrom
rs/fix-memory-leaks
Aug 6, 2026
Merged

fix: a couple of memory leaks#273
imor merged 1 commit into
masterfrom
rs/fix-memory-leaks

Conversation

@imor

@imor imor commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@imor
imor requested a review from steve-chavez August 3, 2026 04:52
@steve-chavez

Copy link
Copy Markdown
Member

Comparing the memory consumption on the loadtests:

There doesn't seem to be a difference 🤔. So looks like there's no real "leak" here because the MemoryContext is already freeing the allocated memory?

That being said, I don't see why not do this and free the memory earlier but then this should be a refactor: commit?

@imor WDYT?

@imor

imor commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Comparing the memory consumption on the loadtests:

There doesn't seem to be a difference 🤔. So looks like there's no real "leak" here because the MemoryContext is already freeing the allocated memory?

That being said, I don't see why not do this and free the memory earlier but then this should be a refactor: commit?

@imor WDYT?

Sorry I should have explained better in the description.

It is a valid leak, just not in the background worker. It's not captured in the loadtests because they look at the memory of the background worker, while the actual leak is in the backend process which calls _urlencode_string and _encode_url_with_params_array via the http_get calls. Memory context can't free this memory because it's not palloced but malloced in libcurl. For this reason I don't think this should be treated as a refactor. If you want I can try to capture the memory usage of the client backend process in loadtests but IMO it's clear enough by statically analyzing the code that this is a memory leak and a test proving it is kinda overkill.

@imor
imor changed the base branch from rs/do-not-download-all-pg-versions to master August 5, 2026 04:28
@imor
imor force-pushed the rs/fix-memory-leaks branch from 2014d83 to e861c0c Compare August 5, 2026 04:31

@steve-chavez steve-chavez left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@imor Thanks for elaborating, I agree it's a fix.

@imor
imor merged commit 68ef0bf into master Aug 6, 2026
19 of 20 checks passed
@imor
imor deleted the rs/fix-memory-leaks branch August 6, 2026 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants