Add optional runtime swappable custom allocator#2230
Add optional runtime swappable custom allocator#2230schloing wants to merge 2 commits intonanomsg:mainfrom
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a configurable custom memory allocation feature via the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/nng/nng.h`:
- Around line 554-556: The declaration of nng_alloc_set is currently guarded by
private macro NNG_ENABLE_CUSTOM_ALLOC which can hide the public API; always
expose the nng_alloc_set declaration (remove the conditional) and implement a
stub/error fallback in the library when custom alloc is not enabled, or
alternatively move the enabling macro into the generated public config and use
that public macro for the guard so consumers and tests see consistent
declarations; update the implementation (function body) to either call the
provided pointers when enabled or return/abort with a clear error when custom
alloc is not supported, ensuring tests can still link against the symbol.
In `@src/nng.c`:
- Around line 51-66: The allocator setters (nng_alloc_set) currently update
nni_malloc_fn, nni_calloc_fn, and nni_free_fn independently and without
synchronization while platform allocators (posix_alloc.c and win_thread.c) read
them unsafely; change nng_alloc_set to be a one-shot, atomic installation that
must be called before any NNG allocations: require the caller to install all
three callbacks together, make nng_alloc_set return an error code (e.g., int) if
called after initialization/allocation has started, and perform a single atomic
swap or guarded check to set all three function pointers at once (or reject the
call). Alternatively, embed the matching free callback into each allocation path
(store the free pointer with the allocation metadata) so that free uses the
allocator that performed the allocation; update references to
nni_malloc_fn/nni_calloc_fn/nni_free_fn accordingly and ensure posix_alloc.c and
win_thread.c use the new safe mechanism.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b2ade47-e511-44b6-97b2-435e051df56e
📒 Files selected for processing (7)
CMakeLists.txtcmake/NNGOptions.cmakeinclude/nng/nng.hsrc/core/defs.hsrc/nng.csrc/platform/posix/posix_alloc.csrc/platform/windows/win_thread.c
| #ifdef NNG_ENABLE_CUSTOM_ALLOC | ||
| NNG_DECL void nng_alloc_set(void* (*malloc)(size_t), void* (*calloc)(size_t, size_t), void (*free)(void*)); | ||
| #endif |
There was a problem hiding this comment.
Don't hide a public API behind NNG_ENABLE_CUSTOM_ALLOC.
With the current wiring, NNG_ENABLE_CUSTOM_ALLOC is private to nng and always public on nng_testing, so enabled builds can hide nng_alloc_set() from real consumers while OFF builds still expose it to tests. This declaration needs to be always visible with a stub/error implementation behind it, or the macro needs to come from a generated public config header that is exported to consumers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/nng/nng.h` around lines 554 - 556, The declaration of nng_alloc_set
is currently guarded by private macro NNG_ENABLE_CUSTOM_ALLOC which can hide the
public API; always expose the nng_alloc_set declaration (remove the conditional)
and implement a stub/error fallback in the library when custom alloc is not
enabled, or alternatively move the enabling macro into the generated public
config and use that public macro for the guard so consumers and tests see
consistent declarations; update the implementation (function body) to either
call the provided pointers when enabled or return/abort with a clear error when
custom alloc is not supported, ensuring tests can still link against the symbol.
| #ifdef NNG_ENABLE_CUSTOM_ALLOC | ||
| void | ||
| nng_alloc_set(void* (*malloc)(size_t), void* (*calloc)(size_t, size_t), void (*free)(void*)) | ||
| { | ||
| if (malloc) { | ||
| nni_malloc_fn = malloc; | ||
| } | ||
|
|
||
| if (calloc) { | ||
| nni_calloc_fn = calloc; | ||
| } | ||
|
|
||
| if (free) { | ||
| nni_free_fn = free; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.c" | grep -E "(posix_alloc|win_thread|nng\.c)" | head -20Repository: nanomsg/nng
Length of output: 137
🏁 Script executed:
# Check the exact location of src/nng.c and the nng_alloc_set function
fd "nng\.c$" --type fRepository: nanomsg/nng
Length of output: 64
🏁 Script executed:
# List the structure to understand the layout
ls -la src/ 2>/dev/null | head -20Repository: nanomsg/nng
Length of output: 707
🏁 Script executed:
# Read src/nng.c around lines 51-66 to see the nng_alloc_set function
sed -n '1,80p' src/nng.c | cat -nRepository: nanomsg/nng
Length of output: 2379
🏁 Script executed:
# Search for the global function pointer declarations
rg "nni_malloc_fn|nni_calloc_fn|nni_free_fn" src/nng.c -B 2 -A 2 -nRepository: nanomsg/nng
Length of output: 225
🏁 Script executed:
# Check posix_alloc.c to see how globals are used
cat -n src/platform/posix/posix_alloc.cRepository: nanomsg/nng
Length of output: 1365
🏁 Script executed:
# Check win_thread.c to see allocator usage
sed -n '1,100p' src/platform/windows/win_thread.c | cat -nRepository: nanomsg/nng
Length of output: 2973
🏁 Script executed:
# Check core/nng_impl.h or core/defs.h for declarations and synchronization
grep -r "nni_malloc_fn\|nni_calloc_fn\|nni_free_fn" src/core/ -B 2 -A 2 -nRepository: nanomsg/nng
Length of output: 463
🏁 Script executed:
# Search for any mutex or synchronization around allocator setting
rg "nng_alloc_set|nni_malloc_fn|nni_calloc_fn|nni_free_fn" src/ --type c -B 3 -A 3 | head -100Repository: nanomsg/nng
Length of output: 4318
🏁 Script executed:
# Check if there's any initialization guard or documentation
rg "ENABLE_CUSTOM_ALLOC" src/ -B 5 -A 5 | head -150Repository: nanomsg/nng
Length of output: 1662
🏁 Script executed:
# Check if there's any documentation about intended usage or timing
grep -r "nng_alloc_set\|custom.*alloc" src/ --type c --type h -B 2 -A 2 | grep -i "comment\|doc\|usage\|before\|after" | head -30Repository: nanomsg/nng
Length of output: 172
🏁 Script executed:
# Check if there are any thread synchronization mechanisms or atomic operations used
rg "atomic|mutex|lock|synchroni" src/core/defs.h -B 3 -A 3Repository: nanomsg/nng
Length of output: 310
🏁 Script executed:
# Verify that the globals are truly not protected by any locks
rg "static.*nni_malloc_fn|static.*nni_calloc_fn|static.*nni_free_fn" src/ -B 5 -A 5Repository: nanomsg/nng
Length of output: 37
🏁 Script executed:
# Check if there's any documentation about nng_alloc_set in header files
rg "nng_alloc_set" src/ --type c --type h -B 5 -A 5Repository: nanomsg/nng
Length of output: 389
🏁 Script executed:
# Look for any public header that might document expected usage
fd "\.h$" src/ -x grep -l "nng_alloc_set" {}Repository: nanomsg/nng
Length of output: 37
🏁 Script executed:
# Check if initialization happens before any allocations
rg "nng_init|nng_open_pipe|nng_socket" src/nng.c -A 5 | head -50Repository: nanomsg/nng
Length of output: 966
🏁 Script executed:
# Look for README or documentation files mentioning custom allocators
fd -e md -e rst -e txt . | xargs grep -l "custom.*alloc\|nng_alloc_set" 2>/dev/nullRepository: nanomsg/nng
Length of output: 39
🏁 Script executed:
# Check git history or comments in the file for intent
head -100 src/nng.c | grep -A 10 -B 10 "NNG_ENABLE_CUSTOM_ALLOC"Repository: nanomsg/nng
Length of output: 396
🏁 Script executed:
# Verify if allocations happen within socket/pipe creation (confirming early initialization would be required)
rg "nni_alloc|nni_zalloc" src/core/ --type c | head -30Repository: nanomsg/nng
Length of output: 2114
🏁 Script executed:
# Double-check: Are the globals truly extern in defs.h, confirming they are shared across compilation units?
cat -n src/core/defs.h | sed -n '245,260p'Repository: nanomsg/nng
Length of output: 732
🏁 Script executed:
# Verify there is no initialization guard or one-shot check anywhere
rg "nng_alloc_set|nni_malloc_fn.*=" src/nng.c -B 10 -A 10Repository: nanomsg/nng
Length of output: 401
🏁 Script executed:
# Check if there's any documentation or comments about thread-safety in platform headers
cat -n src/core/platform.h | grep -A 10 -B 10 "alloc"Repository: nanomsg/nng
Length of output: 4921
🏁 Script executed:
# Final verification: confirm multiple definitions in different compilation units
rg "void\* \(\*nni_malloc_fn\)" src/ --type cRepository: nanomsg/nng
Length of output: 258
🏁 Script executed:
# Verify no atomic or synchronization wrapper exists around the setter
rg "static|atomic|volatile|lock|mutex" src/nng.c | grep -A 5 -B 5 "nng_alloc_set"Repository: nanomsg/nng
Length of output: 37
Synchronize and restrict allocator changes to one-shot initialization.
The globals nni_malloc_fn, nni_calloc_fn, and nni_free_fn are read without synchronization on every alloc/free in src/platform/posix/posix_alloc.c (lines 30, 36, 43) and src/platform/windows/win_thread.c (lines 52, 58, 65). Calling nng_alloc_set() after NNG allocations have begun risks freeing old allocations through a different free callback than the one used for allocation, causing heap mismatch. The independent, partial updates (each function pointer checked separately for NULL) worsen the race window. The void return type prevents callers from detecting or reacting to unsafe timing. Require allocator installation before any NNG allocation occurs, or embed the matching free callback with each allocation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/nng.c` around lines 51 - 66, The allocator setters (nng_alloc_set)
currently update nni_malloc_fn, nni_calloc_fn, and nni_free_fn independently and
without synchronization while platform allocators (posix_alloc.c and
win_thread.c) read them unsafely; change nng_alloc_set to be a one-shot, atomic
installation that must be called before any NNG allocations: require the caller
to install all three callbacks together, make nng_alloc_set return an error code
(e.g., int) if called after initialization/allocation has started, and perform a
single atomic swap or guarded check to set all three function pointers at once
(or reject the call). Alternatively, embed the matching free callback into each
allocation path (store the free pointer with the allocation metadata) so that
free uses the allocator that performed the allocation; update references to
nni_malloc_fn/nni_calloc_fn/nni_free_fn accordingly and ensure posix_alloc.c and
win_thread.c use the new safe mechanism.
gdamore
left a comment
gdamore
left a comment
There was a problem hiding this comment.
There are a few things here:
- This shouldn't be conditionalized in CMAKE. We can do it all the time. Compared to the overhead of actually allocating memory, the function pointer dereference is nothing in terms of performance.
- The allocators should be moved to an nng_init parameter, without a separate function. Most notably it would be very bad for the application to change the allocators during a run -- this could also have bad ramifications if you wind up having for example two different libraries, both running on NNG, but doing this separately (thereby stomping on each other.)
Originally I assumed that changing the allocators would be done as part of a platform port, but I can see other uses here, so I'm supportive of this change in general. Thanks for contributing!
2 participants
Ability to override allocation functions with function pointers
nni_malloc_fn,nni_calloc_fnandnni_free_fn. These pointers can be updated at runtime usingnng_alloc_set.By default, they are set to
malloc,callocandfree. Existing NNG API behaviour not impacted.Functionality disabled by default, and is enabled with CMake option
NNG_ENABLE_CUSTOM_ALLOC. WhenNNG_ENABLE_CUSTOM_ALLOCis off,the function pointers are not used, and
nni_alloc,nni_zalloc, andnni_freecall the corresponding OS functions directly.Summary by CodeRabbit