[Bug] Shared MemoryLimitController silently disables the client memory limit and drops the client memory metrics
- Dominant language
- Java
- Stars
- 15.3k
- Forks
- 3.8k
- Avg merge
- 1d 14h
- Merged PRs (30d)
- 160
Description
### Search before asking
- [x] I searched the [issues](https://github.com/apache/pulsar/issues) and found nothing similar. The *feature* — sharing a `MemoryLimitController` across `PulsarClient` instances — already exists (#25212, closed by #25477). This issue is about defects in that shipped feature.
### Problem
`PulsarClientSharedResources` lets several `PulsarClient` instances share one `MemoryLimitController` (PIP-234, #25477). Three defects make it a footgun rather than a safety mechanism:
**A. Sharing resources silently disables the memory limit.** The shared limit defaults to `0` = unlimited, and an unconfigured shared-resources instance shares *everything*, including the memory limit controller. So the documented "share all resources" snippet silently removes the 64 MiB default limit from every client using it — in exactly the many-clients scenario where a bound matters most.
**B. Per-client `ClientBuilder.memoryLimit(...)` is silently ignored when a controller is injected.** No warning, no exception, no validation. Two clients declaring different limits while sharing: both values are discarded.
**C. The two client memory metrics disappear, or land on the wrong meter provider.**
### Concrete evidence
**A —** `PulsarClientSharedResourcesBuilderImpl.java:209-210`:
```java
static class MemoryLimitResourceConfig implements ResourceConfig, MemoryLimitConfig {
long memoryLimit; // defaults to 0
```
`0` means unlimited (`MemoryLimitController.isMemoryLimited()` → `memoryLimit > 0`, `MemoryLimitController.java:153-155`), and an unconfigured builder shares every resource type (`PulsarClientSharedResourcesImpl.java:70-71`, `EnumSet.allOf(SharedResource.class)`), which includes `SharedResource.MemoryLimitController` (`:103-106`). The injected controller then wins unconditionally — `PulsarClientImpl.java:347-354`:
```java
if (memoryLimitController == null) {
this.memoryLimitController = new MemoryLimitController(conf.getMemoryLimitBytes(), ...);
} else {
this.memoryLimitController = memoryLimitController; // conf.getMemoryLimitBytes() never read
this.memoryLimitController.registerTrigger(this.memoryLimitTrigger);
}
```
So `ClientConfigurationData.java:436` (`memoryLimitBytes = 64 * 1024 * 1024`) is discarded. **The affected snippet is the documented usage example** in `PulsarClientSharedResources.java` ("To share all possible resources across multiple PulsarClient instances" → `builder().build()`), and `PulsarClientSharedResourcesBuilderImplTest` exercises exactly that shape across 1000 clients while asserting nothing about the limit — `rg 'MemoryLimit|memoryLimit'` over that test file returns **zero** matches.
**B —** same `PulsarClientImpl.java:347-354`. Neither `ClientBuilder.memoryLimit(...)` nor `ClientBuilder.sharedResources(...)` javadoc mentions the interaction, and nothing validates a conflict.
**C —** `PulsarClientImpl.java:356-360`:
```java
// Only create memory buffer metrics if memory limit controller is local and memory limiting is enabled.
if (memoryLimitController == null && this.memoryLimitController.isMemoryLimited()) {
this.memoryBufferStats = new MemoryBufferStats(this.instrumentProvider, this.memoryLimitController);
} else {
this.memoryBufferStats = null;
}
```
The condition keys off the **injected constructor parameter**, so any shared controller suppresses the per-client registration of `pulsar.client.memory.buffer.usage` and `pulsar.client.memory.buffer.limit` (`metrics/MemoryBufferStats.java:26-30`). The compensating registration in `PulsarClientSharedResourcesImpl.java:112-117` requires **both** a non-zero shared limit **and** `SharedResource.OpenTelemetry` in the shared set. Outcomes:
- Shared controller left at its `0` default → neither side registers → **both metrics silently vanish.**
- `SharedResource.OpenTelemetry` not shared (e.g. an explicit resource list, or `shareConfigured()` with only `configureMemoryLimitController(...)`) → `instrumentProvider == null` (`:107-110`) → **both metrics silently vanish**, even with a non-zero shared limit.
- Everything shared, limit configured, but OTel unconfigured → the shared `InstrumentProvider` falls back to `GlobalOpenTelemetry.get()` (`InstrumentProvider.java:38-43`), so these two metrics go to the global instance while **every other client metric** goes to the SDK passed to `ClientBuilder.openTelemetry(sdk)`.
The last case is an API gap, not a missing line. `PulsarClientSharedResourcesImpl.applyTo` (`:168-193`) sets 8 things and `instrumentProvider` is not among them, and it *cannot* be: the Lombok `@Builder` sits on the `PulsarClientImpl` constructor, whose parameter list has no `instrumentProvider`, so `PulsarClientImplBuilder` has no such setter. `PulsarClientImpl.java:275` unconditionally does `new InstrumentProvider(conf.getOpenTelemetry())`. Consequently `SharedResource.OpenTelemetry` affects **only** the shared `memoryBufferStats` and nothing else — and repo-wide, `configureOpenTelemetry(...)` and `PulsarClientSharedResourcesImpl.getInstrumentProvider()` have **zero callers**, so nothing today depends on the current behaviour.
`MemoryLimitConfig`'s javadoc reinforces the mismatch: it says "See also `ClientBuilder#memoryLimit(long, SizeUnit)`", reading as a shared analogue of the client-level setting, while in fact it silently overrides it and only its non-zero form keeps the metrics alive.
### Reachability
No broker or network needed. `PulsarClientSharedResources.builder().build()` + `PulsarClient.builder().sharedResources(shared)` is sufficient to observe A and C; assert on `((PulsarClientImpl) client).getMemoryLimitController().isMemoryLimited()` and on the absence of the two metrics. Existing coverage (`ProducerMemoryLimitTest.testMultiPulsarClientProducerShareMemoryLimitController`, `ConsumerMemoryLimitTest.testMultiPulsarClientConsumerShareMemoryLimitController`) only covers the *configured, non-zero* path.
### Proposed solution
**For A** — pick one; this needs a call:
1. Default the shared limit to `ClientConfigurationData`'s 64 MiB instead of `0`. Consistent with the per-client default; changes behaviour for anyone relying on today's accidental "unlimited".
2. Require an explicit `configureMemoryLimitController(...)` whenever `SharedResource.MemoryLimitController` is in the shared set, and fail the build otherwise. Loudest and safest; breaks the documented `builder().build()` example.
3. Exclude `MemoryLimitController` from the share-everything default so unconfigured sharing leaves each client's own limit intact. Least disruptive, but makes "all" not actually mean all.
**For B** — reject or warn on a conflict: if a client sets `memoryLimit(...)` explicitly *and* is given a shared controller, log a warning (or fail the build) instead of discarding the value silently. Document the precedence on both `ClientBuilder.memoryLimit` and `ClientBuilder.sharedResources`.
**For C** — register the two metrics exactly once regardless of the sharing shape, and route them through the same `InstrumentProvider` as the rest of the client's metrics. The straightforward version adds `instrumentProvider` to `PulsarClientImpl`'s builder and passes the shared one through `applyTo` — a (package-private) API change, hence a design decision rather than a patch. A narrower fix: register `MemoryBufferStats` on the shared object whenever the shared controller is memory-limited, independent of whether `SharedResource.OpenTelemetry` is shared.
Fixing A is a prerequisite for the broker/proxy work in #26346: wiring the broker onto a shared controller while that controller silently defaults to unlimited would achieve nothing.
### Scope & compatibility
- **A** changes observable behaviour for anyone already sharing resources without configuring a limit. Framed as a bug fix (the current behaviour silently discards a documented default), but options 1 and 2 cross into "semantics of existing functionality" — flag in release notes; a PIP is arguably warranted for option 2, since it makes an existing documented snippet throw.
- **B** is validation plus javadoc — bug-fix scope.
- **C** is a bug fix if it only restores the two existing metric names on the shared path; it needs a **PIP** if it adds public API (propagating a shared `InstrumentProvider` into clients).
- No wire-protocol, metadata-format or client-server compatibility impact. The two metric names are unchanged in every option.
### Related
- #25212 (closed, completed) / #25477 (merged) — PIP-234 shared `MemoryLimitController`; this issue reports defects in that feature, not the feature itself.
- #19074 — PIP-234 umbrella; #24790 — shared thread pools and DNS resolver; #24893 / #24796 — shared resources in `PulsarAdmin`; #25072 / #24795 — shared resources in `AuthenticationOAuth2`.
- #18938, #18939 — memory-limit-controller accounting leaks on consumer close / `clearIncomingMessages` (different bug, non-overlapping).
- #26340 / #26341 / #26342 — the client memory-limit default work that surfaced this area.
- #26346 — configurable memory limit for broker, proxy and WebSocket proxy clients (depends on this).
Contributor guide
Assessment
This issue has not been assessed yet.