Repository navigation
fix: free value buffer, data struct and async work in the async path - #48
Open
guillaume-flambard wants to merge 1 commit into
Open
guillaume-flambard wants to merge 1 commit into
guillaume-flambard wants to merge 1 commit into
Conversation
Every async call leaked the value buffer (get/list), the data struct on error branches, and the napi_async_work handle (never deleted). The sync path was already correct. - keep the async work handle in the per-call struct so it can be deleted - delete the async work and free all buffers/struct on every exit path - zero-initialise the per-call struct so failed size probes do not leave garbage pointers behind Regression test: 300k async set/get/remove calls bounded at 20 MB rss growth (fails at ~150 MB without the fix).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #47. Also fixes the leak reported against storage-api in supabase/storage#1349.
Credit for the diagnosis goes to @djonua: #47 pins the exact faulty lines, isolates the bug against the sync path, and carries the production numbers. This PR implements the fix outlined there.
What leaked on every async call:
valuebuffer inxattr_get_executeand theresultbuffer inxattr_list_executewere never freednapi_async_workhandle lived in a function-local andnapi_delete_async_workwas never called, so one work object leaked per callChanges, same shape across all four operations:
calloc, so a failed size probe cannot leave an uninitialized pointer behindRegression test: 300k async set/get/remove calls, bounded at 20 MB of rss growth. On the unfixed build it fails at ~150 MB (about 0.5 kB per call, matching the numbers in #47). With the fix it passes. A direct 200k async
getrun on this machine measured 7.5 MB total growth (0.04 kB per call).Happy to adjust if you would rather see the struct freed before the work is deleted, or any other ordering.