Header pooling

[danielmarbach]

What this does

Pools the Dictionary<string, string> instances we allocate for message headers. A new DictionaryPool<TKey, TValue> lives in NServiceBus.Utils, and HeaderPool in NServiceBus.Transport is a thin subclass with header-tuned defaults. We rent on the produce side (send, publish, reply, the audit fork, the multicast copy) and return after dispatch.

The learning transport pump rents its incoming headers too, and HeaderSerializer now populates a rented dictionary instead of allocating one. That needed a manual Utf8JsonReader parse, because System.Text.Json always allocates a fresh dictionary and can't fill a rented one.

Why

Header dictionaries are a large chunk of what we allocate per message. Reusing them via Clear() keeps the internal entry and bucket arrays, so the next rent skips the resizes that dominate header-copy traffic.

I want to be honest about the shape of the evidence. On a representative run (in-memory transport, one downstream send per message, no fanout inflation): header entry arrays about halved per message, total per-message allocation down ~14%, time in GC down from 11.1% to 9.6%, throughput up ~7%.

I don't think the in-memory harness can tell us what a real endpoint looks like. It has no handler work and no real transport I/O, so it can't represent a handler's allocation mix or the async semantics of a real transport. The framework per-message saving is what's measured; how large a share of a production endpoint's total that is depends on the handler. I'd like us to validate on a real transport before calling this done.

Why ConcurrentStack over ConcurrentBag or an interlocked fast-slot

Three options, briefly:

• ConcurrentBag (first attempt). Per-core work-stealing queues backed by a monitor. Once more sites pooled, it spent about 12.7% of CPU in Monitor.Enter_Slowpath and throughput dropped with it. The monitor is the problem.
• A thread-affine slot: ThreadStatic, or a single interlocked fast-slot with a stack overflow. This is the ArrayPool.Shared pattern, and it doesn't carry over. Rent and return straddle an await: we rent on the pump or handler thread and return on a dispatch continuation thread, so there is no thread-local locality to exploit. A ThreadStatic slot can't hand a dictionary back to its renter, and on a real transport it would fragment (returns pile up on dispatch threads, renters allocate fresh). A single interlocked fast-slot is just another hot CAS line every thread hammers, with no locality payoff, so the fast-slot-plus-overflow shape is complexity without benefit here. The in-memory transport hid all of this because its dispatch completes synchronously, which a real transport's async I/O does not.
• ConcurrentStack (chosen). Lock-free CAS, no monitor. The pool no longer shows up in the monitor profile, and every guarantee carries over from the bag version (soft cap, capacity preserved by Clear, TrimExcess on oversized, minimumCapacity/EnsureCapacity, the clearDictionary opt-out, null guard). One container, same API, same tests.

Minor or major?

Most of this is additive. DictionaryPool<TKey, TValue>, HeaderPool, and the CopyTo extension are new public types, so they're safe for a minor.

The one real question is the behavioral change. After dispatch, header dictionaries are now cleared and returned to the pool. Before this, they survived dispatch as orphaned objects you could still read. Reading headers after the pipeline completes was never part of the contract: the pipeline owns the header lifecycle for the duration of execution, and nothing in the docs promises the dictionaries stay populated afterward.

My call is minor, with a release note. The reasoning: the only code this affects is code that held a header reference and read it after dispatch, which "happened to work" but was never promised. A release note saying header dictionaries are pooled and cleared after dispatch, and that reading them after the pipeline completes is outside the contract, covers it. I don't think this rises to a major. If we treated any observable change to undocumented behavior as a major, we'd rarely be able to ship anything.

The audit backs this up. The only code that can observe the change is code that reads header references after dispatch, and nothing in the platform does that. Every transport reads headers only synchronously during Dispatch and never retains them, and every production outbox hands out freshly deserialized dictionaries. That retires the "wide, partly-un-auditable blast radius" argument for shipping this opt-in. I'd still be fine with opt-in in the minor and default-on in the next major if reviewers prefer the conservative path, but the justification for that is now "observable-but-undocumented behavior change plus not yet validated on a real transport", not "we can't audit the blast radius".

Two smaller things that don't affect the call. The learning transport's HeaderSerializer.Deserialize can now throw JsonReaderException (a subclass of JsonException) instead of JsonException on malformed input. It's internal and dev/test only, and the wire format that Serialize produces is unchanged. The manual parser is an internal learning-transport detail, not public surface.

Outbox contract worth flagging

ImmediateDispatchTerminator returns every dispatched header dictionary to the pool after dispatch, including ones it didn't rent (same semantics as ArrayPool). That's fine as long as outbox Get hands out dictionaries it owns. A real outbox does, because it deserializes fresh from storage. The in-memory acceptance-testing outbox didn't: it returned shared references, and dispatch cleared and pooled them. AcceptanceTestingOutboxStorage.Get now copies, the same way the non-durable persistence does.

I ran a blast-radius audit across the platform to check this properly: all 8 first-party outbox implementations (Sql, MongoDB, NHibernate, CosmosDB, AzureTable, DynamoDB, AcceptanceTesting, NonDurable) and all 11 transport, bridge, gateway, and transactional-session repos. Every production outbox deserializes fresh header dictionaries on Get, and every transport reads headers only synchronously during Dispatch, then never touches the dictionary again.

Two in-memory outboxes violated the contract and are fixed. AcceptanceTestingOutboxStorage.Get and FakeOutboxStorage.Get (Core.Tests) now copy per call, and NonDurableOutboxStorage.Get has the same fix in Persistence.NonDurable as a companion change, not in this PR.

If someone ships an IOutboxStorage whose Get returns shared header references, it will break under pooling. Worth a line in the outbox contract.

What's in the PR

• New public API: DictionaryPool<TKey, TValue>, HeaderPool, CopyTo extension. API approvals updated.
• Rent sites: MessageOperations, RoutingContextExtensions (multicast), InvokeAuditPipelineBehavior (audit).
• Return sites: ImmediateDispatchTerminator, RoutingToDispatchConnector (multicast original).
• Learning transport pump and HeaderSerializer.
• DictionaryPoolTests (9) and HeaderSerializerTests.

Open

Not validated on a real transport yet, that's the remaining gap before I'd call this done. The audit evidence is file:line referenced and consistent, but I'd like a human spot-check before we lean on it for a release decision. Happy to drop the learning-pump changes if they feel out of scope here, and happy to revert anything that doesn't hold up. Please don't hesitate to challenge the backing choice or the numbers.

[danielmarbach]

I'm also totally OK in case we decide the "behavior" break is not worth the risk in case someone retains references to headers when the message handling pipeline is already completed.

FYI RabbitMQ client and SQS transport pools memory and returns after the message is completed so that user segment would already not be capable of reusing things outside the handling pipeline without copying. Azure ServiceBus,SQL and Storage do not pool as of today.

[danielmarbach]

One thing I'd like explicit input on: how we ship this.

The audit (now in the body) retired the "we can't audit the blast radius" argument for opt-in. Every production outbox hands out freshly deserialized header dictionaries on Get, and every transport reads headers only synchronously during Dispatch, so nothing in the platform observes the post-dispatch clearing. What's left is the observable-but-undocumented behavior change, plus the fact that we haven't validated on a real transport yet.

So the options as I see them:

• Opt-in in the minor, default-on in the next major. Most conservative. Cost: a settings knob we'd later deprecate, and one release of unrealized perf benefit.
• Default-on in the minor. Defensible under our source-level SemVer if we release-note the behavioral change. Immediate win, no knob.
• Default-on with a documented opt-out escape hatch. The middle ground: immediate win, and a survivable valve if a regression surfaces.

I don't think this is a correctness question anymore. What's left is how much risk we want to take on, and I'd rather hear your take than decide on my own.

[danielmarbach]

Rebased on latest trimming work

[andreasohlund]

Default-on in the minor. Defensible under our source-level SemVer if we release-note the behavioral change. Immediate win, no knob.

Since we already have at least one transport that doesn't support this anyway, my vote would go for this option

[bording]

If someone ships an IOutboxStorage whose Get returns shared header references, it will break under pooling. Worth a line in the outbox contract.

@danielmarbach If we're saying this pooling behavior is part of the persistence and transport contracts, if possible, I'd like to see some new tests in the PersistenceTests and TransportTests propject that enforce this for all implementations.

Since we already have at least one transport that doesn't support this anyway, my vote would go for this option

@andreasohlund This would only be true for message bodies though right, not the header dictionaries that we are currently handing out?

To clarify, the risk of introducing header pooling is that someone could have a behavior or a handler that is doing something strange and storing the header dictionaries for use outside of a pipeline invocation?

While I do think that would be an unlikely thing to do, it's not clear to me that the benefit here is worth doing in a minor release vs. waiting for the next major version to do it in a more safe manor.