feat: allocator-api and shared memory support #164

bavshin-f5 · 2025-06-17T17:15:32Z

Not quite at the state I wanted to reach, but should be good to start the review.

Known issues and annoyances:

NgxString size is 4 words (ptr, len, capacity, alloc). std::string::String is the same though, so this can be ignored for now.
There's no non-deprecated Hasher impl in ::core. We may need to add another dependency or implement our own Hasher.
The RwLock is just a tiniest bit evil. It supposed to work anywhere where nginx locks do, but I have doubts...
I considered pthread_rwlock_t with pthread_rwlockattr_setpshared, but there's that one platform without POSIX threads.
Some remaining casual unsoundness, esp. around the RbTree. We may consider making pure Rust implementations of the data structures with fallible allocation support as a separate crate in the future.
Windows failure is due to postconfiguration not being invoked. Not reproducible locally with up-to-date toolchain.
Still need to figure how to wrap ngx_shm_zone_t safely.

Copilot

Pull Request Overview

This PR adds allocator-api integration and shared memory support for NGINX modules by introducing new synchronization primitives, allocator-backed string and data structures, and wrappers around NGINX’s slab and pool APIs.

Implement shared‐memory reader/writer locks (sync.rs)
Add allocator‐aware NgxString, RbTree, SlabPool, and Pool types
Integrate allocator-api2, update Cargo features, and add ngx_sched_yield shim

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/sync.rs	RawSpinlock and `RwLock` for interprocess sync
src/lib.rs	Reexport `allocator` and `sync` modules
src/core/string.rs	`NgxString` with alloc support and tests
src/core/rbtree.rs	`RbTree` wrapper using nginx red‐black tree API
src/core/slab.rs	`SlabPool` allocator over `ngx_slab_pool_t`
src/core/pool.rs	`Pool` wrapper over `ngx_pool_t` with Allocator
src/core/mod.rs	Expose new `rbtree` and `slab` modules
src/allocator.rs	`TryCloneIn` trait and allocator helper functions
nginx-sys/src/lib.rs	Add `ngx_sched_yield` fallback implementation

Comments suppressed due to low confidence (3)

src/core/string.rs:580

[nitpick] There is no test for the error path of append_within_capacity. Adding a case where the buffer is full would ensure correct overflow behavior is covered.

    fn test_string_comparisons() {

src/allocator.rs:10

[nitpick] The TryCloneIn trait lacks doc comments on its purpose and usage. Consider adding a brief overview and example in the trait-level doc.

pub trait TryCloneIn: Sized {

nginx-sys/src/lib.rs:181

The call to usleep isn’t imported or qualified, causing a compile error. Consider calling libc::usleep or importing usleep from the proper crate.

        usleep(1)

src/core/rbtree.rs

fitzgen

Hi! @pchickey asked me to look at the Allocator trait bits because I have some experience implementing and using that trait.

The dangling_aligned helper could probably be simplified a little bit if it didn't have a type parameter, and always returned a NonNull<u8>. The only caller I saw (unless I missed something) uses T = u8 and so if the type parameter was removed, then that also removes some of the questions around T's alignment versus the alignment dynamically passed in as an argument, makes some debug_assert!s unnecessary, etc.

The comments about overriding the provided default Allocator methods are really opportunistic and are things that would be nice if the underlying capability is there. (And if the underlying capability isn't there right now, you might want to consider whether it is worth adding in the future. See the comment about building up Vecs.)

But yeah, overall, looks good to me!

(Leaving this review as a "comment" because I don't want to presume myself to be an nginx expert who can authoritatively approve or request changes to PRs or anything like that; just sharing my thoughts after a once-over. Hopefully they are helpful!)

src/allocator.rs

fitzgen · 2025-06-18T19:17:08Z

src/core/pool.rs

+    unsafe fn deallocate(&self, ptr: NonNull<u8>, layout: Layout) {
+        // ngx_pfree is noop for small allocations unless NGX_DEBUG_PCALLOC is set.
+        //
+        // XXX: there should be no cleanup handlers for the allocations made using this API.
+        // Violating that could result in the following issues:
+        //  - use-after-free on large allocation
+        //  - multiple cleanup handlers attached to a dangling ptr (these are not unique)
+        if layout.size() > 0 // 0 is dangling ptr
+            && (layout.size() > self.as_ref().max || layout.align() > NGX_ALIGNMENT)
+        {
+            ngx_pfree(self.0.as_ptr(), ptr.as_ptr().cast());
+        }
+    }


Does ngx_pool_t do non-noop frees if done in LIFO order? If so, then it would also make sense for it to support in-place reallocs for the last allocation, and override the provided default methods for Allocator::grow and Allocator::shrink here as well.

Allowing the last allocation to grow in-place, even in bump allocators, is particularly nice when building Vecs of an unknown number of elements and which end up resizing and growing their underlying storage.

Unfortunately, ngx_pool_t is not capable of that. It does not even track the necessary information.

The pool internals are exposed though, so there's a possibility for implementing this optimization in future. In a fragile and dangerous way :(

fitzgen · 2025-06-18T19:18:25Z

src/core/pool.rs

+pub struct Pool(NonNull<ngx_pool_t>);
+
+unsafe impl Allocator for Pool {
+    fn allocate(&self, layout: Layout) -> Result<NonNull<[u8]>, AllocError> {


It seems like ngx_pcalloc exists, so it probably makes sense to implement Allocator::allocate_zeroed as well.

ngx_pcalloc is a combination of ngx_palloc and memset, the same as the default implementation of Allocator::allocate_zeroed. A correct implementation of Allocator::allocate_zeroed for ngx_pool_t would inherit all of the Allocator::allocate complexity, swap ngx_palloc with ngx_pcalloc and zeroize the data in the remaining branches.
Besides the obvious maintenance cost increase, I assumed that the default Allocator::allocate_zeroed has more potential for inlining and optimizations.

fitzgen · 2025-06-18T19:21:59Z

src/core/slab.rs

+/// Wrapper for a locked [`ngx_slab_pool_t`] pointer.
+pub struct LockedSlabPool(NonNull<ngx_slab_pool_t>);
+
+unsafe impl Allocator for LockedSlabPool {


Similarly, it may make sense to provide non-default implementations of Allocator::{allocate_zeroed,grow,shrink} here as well, if the underlying ngx_slab_pool_t supports those operations (even if only for the last allocation).

Again, not something ngx_slab_pool_t is capable of doing.
It's possible to make grow noop if it fits into the same slot, but that requires reproducing a part of the slab pool internals in Rust.

xeioex

Some minor issues I found up to "feat: owned byte string type with Allocator support"

src/core/pool.rs

src/core/string.rs

pchickey

Really nice work. Handful of pretty minor nits.

pchickey · 2025-06-20T18:34:23Z

examples/shared_dict.rs

+    let args =
+        unsafe { slice::from_raw_parts_mut((*cf.args).elts as *mut ngx_str_t, (*cf.args).nelts) };
+
+    let name: ngx_str_t = args[1];


Please validate args len first

The len is validated by the config parser before invoking the callback. If it doesn't match the flags set in the ngx_command_t, it's totally fine to crash or fail in any other way.

I addressed this in a comment and added an assertion — likely redundant as Index ops are checked in debug.

pchickey · 2025-06-20T19:24:16Z

examples/shared_dict.rs

+            return;
+        };
+
+        let value = unsafe { slice::from_raw_parts(v.data, v.len() as usize) };


This could use a comment that indicates its semantically equivalent to the try_from_bytes_in above

src/allocator.rs

src/core/rbtree.rs

pchickey · 2025-06-20T22:31:58Z

src/core/rbtree.rs

+
+/// Raw iterator over the `ngx_rbtree_t` nodes.
+///
+/// This iterator remains valid after removing the current item from the tree.


I'm not sure if this struct actually needs to be pub - it looks to me like its private to the implementation of all the other structs, but maybe I missed something. It looks like the two places this gets used its paired with a PhantomData<(K, V)> so maybe that aspect can be folded into the "Raw" iter?

The second line in the dod comment needs elaboration. What is the current item in the tree, the Item? What is the operation to remove an Item from the tree? Maybe the comment about validity to perform some specified remove operation actually belongs on the methods for RBTree::iter and RBTree::iter_mut, and on Iter and IterMut, if it matters to the consumers of those structs/methods.

After this comment I realized that I had no clear picture when writing ngx::core::rbtree, and thus failed in documenting the purpose.

The underlying tree is not quite a complete key-value map implementation. Sure, there is a key: ngx_rbtree_key_t (a.k.a. usize), and a value type which embeds the ngx_rbtree_node_t, but RbTree<K,V,A> type is not a safe wrapper for an arbitrary NGINX rbtree. It is an implementation of a Map<K,V> with ngx_rbtree_t as a building block.
RawIter though works on any ngx_rbtree_key_t and just accesses the tree nodes sorted by node.key, not being aware of the value type or allocation method.

In the end, I want to implement both a generic ngx_rbtree_t wrapper to work with references to the NGINX internal trees, and a high-level Map/Set/etc type for a pure Rust module code. I renamed things and added some comments to clarify that, but I don't believe I got the abstractions right yet.
I'll take some time to think about that while attending the conference.

The current implementation should be good enough for our internal projects though.

src/core/rbtree.rs

pchickey · 2025-06-20T22:38:43Z

src/core/rbtree.rs

+    _ph: PhantomData<(K, V)>,
+}
+
+struct Node<K, V> {


I may be misunderstanding exactly what this private helper is used for (it should have a comment) but if the offset_of the node field is important, should this be #[repr(C)] to make sure the offset of node matches the C repr? It looks like the Layout of this gets passed to the allocator, do we need to guarantee that it has the same layout as the C repr?

This type has no C counterpart and will not be accessed from C. AFAIU, the field offsets are stable when used within the same Rust compilation target, notwithstanding the layout or repr, so we don't need to force a specific representation.

src/sync.rs

src/core/string.rs

Add some helpers for working with allocators.

bavshin-f5 · 2025-06-21T03:33:41Z

Appreciate the very detailed reviews.
Addressed most of the small nits with the last push, will take a pause to figure out how to improve the RbTree interface.

bavshin-f5 force-pushed the allocators branch from ec90152 to c7eba70 Compare June 17, 2025 18:50

bavshin-f5 requested a review from Copilot June 17, 2025 21:00

Copilot AI reviewed Jun 17, 2025

View reviewed changes

src/core/rbtree.rs Outdated Show resolved Hide resolved

bavshin-f5 force-pushed the allocators branch 3 times, most recently from c01f18a to a73f48b Compare June 18, 2025 17:05

fitzgen reviewed Jun 18, 2025

View reviewed changes

bavshin-f5 force-pushed the allocators branch from f6b6267 to f73cd93 Compare June 19, 2025 02:28

xeioex suggested changes Jun 19, 2025

View reviewed changes

src/core/pool.rs Outdated Show resolved Hide resolved

src/core/string.rs Show resolved Hide resolved

src/core/string.rs Outdated Show resolved Hide resolved

bavshin-f5 force-pushed the allocators branch from f73cd93 to c184d27 Compare June 20, 2025 01:25

pchickey reviewed Jun 20, 2025

View reviewed changes

bavshin-f5 added 8 commits June 20, 2025 19:50

feat: add and reexport allocator-api2

fb1b1e0

Add some helpers for working with allocators.

feat: implement Allocator for Pool

5e337b9

feat: wrapper for ngx_slab_pool_t

277e183

feat: owned byte string type with Allocator support

9e251c4

feat: ngx::sync::RwLock implementation

e15a910

feat: Map implementation based on the ngx_rbtree_t

2e96b09

feat(sys): implement as_bytes() on variable values

32065f2

feat(examples): shared memory usage example

894cff6

bavshin-f5 force-pushed the allocators branch from c184d27 to 894cff6 Compare June 21, 2025 02:55

bavshin-f5 mentioned this pull request Jun 21, 2025

Shared memory synchronization primitives #126

Open

pchickey approved these changes Jun 23, 2025

View reviewed changes

feat: allocator-api and shared memory support #164

Are you sure you want to change the base?

feat: allocator-api and shared memory support #164

Uh oh!

Conversation

bavshin-f5 commented Jun 17, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xeioex left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pchickey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bavshin-f5 commented Jun 21, 2025

Uh oh!

Uh oh!