Skip to content

Release messageRouter persistent reference in ProducerConfig destructor (per-producer GC leak) - #511

Open
amit-motorq wants to merge 1 commit into
apache:masterfrom
amit-motorq:fix/message-router-persistent-reference-leak
Open

amit-motorq wants to merge 1 commit into
apache:masterfrom
amit-motorq:fix/message-router-persistent-reference-leak

Conversation

@amit-motorq

Copy link
Copy Markdown

Problem

When a producer is created with a custom messageRouter, ProducerConfig stores the user's JS callback in a Napi::FunctionReference created with Napi::Persistent (src/ProducerConfig.cc):

messageRouter = Napi::Persistent(value.As<Napi::Function>());
pulsar_producer_configuration_set_message_router(this->cProducerConfig.get(), choosePartition, &messageRouter);

Persistent handles are GC roots: the referenced JS function — and everything its closure captures — can never be garbage-collected for as long as the handle exists. Producer deliberately extends the ProducerConfig lifetime (comment in src/Producer.h: "Extend the lifetime of the producer config since it's env and router function could be used when sending messages"), but nothing ever releases the reference: not Producer::Close(), not ~Producer(), and not ~ProducerConfig() — which is empty:

ProducerConfig::~ProducerConfig() {}

Consequence: any application that closes producers and creates new ones (periodic producer recycling for config/credential rotation, partition-count refresh, connection recovery, etc.) accumulates unreachable-but-pinned callback graphs forever, one per closed producer. Each pinned graph also keeps the JS Producer/Client ObjectWrap alive, so their finalizers never run — and because N-API addons free their underlying C++ objects from those finalizers, the C++ producer/client allocations are never freed either. The leak therefore presents as steady RSS growth per closed producer while the JS heap stays flat, making it easy to misdiagnose as a native-side leak.

Evidence

Heap snapshots taken from a long-running Node service that recycles producers on a 30-minute TTL (via client.createProducer → producer.close() cycles):

  • The population of closed producers' router closures grew linearly with producer churn (~2 per producer per recycle interval), while the live producer count stayed constant.
  • BFS retaining path for every leaked producer was identical: (GC roots) → (Global handles) → messageRouter → [context] → captured scope → this → producer/client graph — i.e., a native persistent reference that survived close().
  • Total JS heap self-size stayed flat across the window; RSS tracked the number of pinned native producer/client graphs (~150 KB each), confirming the leaked bytes are the C++ allocations behind the pinned ObjectWraps.

Minimal reproduction sketch (needs a broker; run node with --expose-gc):

const Pulsar = require('pulsar-client');

(async () => {
  const client = new Pulsar.Client({ serviceUrl: 'pulsar://localhost:6650' });
  const weakRefs = [];
  for (let i = 0; i < 100; i++) {
    const captured = new Array(1024 * 1024).fill('x'); // 8 MB payload per iteration
    const wr = new WeakRef(captured);
    const producer = await client.createProducer({
      topic: 'persistent://public/default/t',
      messageRoutingMode: 'CustomPartition',
      messageRouter: (msg) => { void captured; return 0; },
    });
    await producer.close();
    weakRefs.push(wr);
  }
  await client.close();
  global.gc();
  // Before this fix: most weakRefs still deref to the captured payload (pinned by the native handle).
  // After this fix: they deref to undefined — closures are collected.
})();

Fix

Call messageRouter.Reset() in ~ProducerConfig():

ProducerConfig::~ProducerConfig() {
  messageRouter.Reset();
}

Safety notes:

  • The destructor runs only when the last shared_ptr<ProducerConfig> is released — i.e., after the Producer wrapper has been collected and its C producer freed — so no routing callback can be invoked afterwards (the extended-lifetime contract in Producer.h is preserved for the entire live lifetime of the producer).
  • Reset() is a no-op for a never-assigned reference (producer created without a custom messageRouter).
  • Destruction order is irrelevant here: freeing the C producer never invokes the router.

Testing

  • No behavior change for any public API; messageRouter semantics during the producer's lifetime are untouched.
  • A deterministic unit test would require running jest under --expose-gc (not enabled in the current npm test script); the WeakRef-based reproduction above verifies the fix when run manually under --expose-gc.
  • Existing producer tests (tests/producer.test.js — custom routing cases) cover that routing still works.

ProducerConfig stores the user's messageRouter JS callback in a
Napi::FunctionReference created with Napi::Persistent. Persistent handles
are GC roots, so the callback — and everything its closure captures — can
never be collected. Producer deliberately extends ProducerConfig's lifetime
(shared_ptr held by the Producer wrapper), but neither Producer::Close nor
~Producer nor ~ProducerConfig ever calls Reset() on the reference.

The result: any application that closes producers and creates new ones
(e.g. periodic recycling of producers for config/credential rotation or
partition-count refresh) accumulates unreachable-but-pinned callback
graphs forever. Each pinned graph also keeps the JS Producer/Client
ObjectWrap alive, so their finalizers never run and the underlying C++
producer/client allocations are never freed either — the leak shows up as
steady RSS growth while the JS heap stays flat.

Reset the reference in ~ProducerConfig. The destructor runs only after
the last shared_ptr is released, i.e. after the Producer wrapper is gone
and its C producer freed, so no routing callback can be invoked afterwards.
Reset() is a no-op for a never-assigned reference (no messageRouter
configured).

Verified with heap snapshots taken before and after producer
create/close cycles: closed producers' router closures were retained via
(GC roots) -> (Global handles) -> messageRouter -> [context] -> this and
grew linearly with churn; with this change they are collected and the
retained set stays at the live producer count.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.

1 participant