Conversation
…lures Motivation Found in the review of apache#26455, on the compaction flush path: - StrategicTwoPhaseCompactor.flushBatchMessage(): the serialized batch buffer is owned solely by the local variable once toByteBuf() returned (the container has already cleared itself), but a failure before the BookKeeper hand-off completes - an interrupted outstanding.acquire(), a throwing stats update, or asyncAddEntry failing synchronously - only discarded the (already empty) container: the serialized buffer leaked on every failed flush. - The outstanding-writes permit taken before such a failure was never returned either (the write callback that owns the release never runs), permanently shrinking the limit until compaction stalls in acquire(). - RawBatchMessageContainerImpl.toByteBuf(): the output buffer is allocated before the header writes, but a failure between the allocation and the return left it orphaned - the finally released the intermediate buffers only. Modifications - flushBatchMessage() releases the serialized buffer when the hand-off never happened (it is nulled after the successful hand-off, when BookKeeper owns it) and returns the permit when it was acquired - an acquire() that itself throws must not release one it never took, or the outstanding-writes limit silently grows. - The write callback's metrics update is caught: the callback can run inline inside asyncAddEntry, and an exception escaping it would reach the caller's catch, releasing the permit and the buffer a second time after the write already took them. With the callback unable to throw, an exception from asyncAddEntry itself can only mean the callback never ran - verified against BookKeeper 4.18.0, whose synchronous throws in doAsyncAddEntry all precede any callback execution - which is exactly the case the caller's catch handles. - toByteBuf() releases the output buffer in its finally as well, nulled on the success path where its ownership moved to the returned buffer; its allocation goes through the instance allocator (the default constructor already uses PulsarByteBufAllocator.DEFAULT), which is what lets the regression test track it. Verification - Regression test: a tracked output buffer whose final write throws must be released (fails with "expected [0] but found [1]" on the previous code). - The compactor fixes have no direct test seam (private flush method, inline container); the reasoning is stated above. Assisted-by: Claude Code
a meta-question: are you running into allocation failures? Perhaps that should be addressed as the primary solution instead of handling all possible code paths where allocation could fail. A recommended setting is to crash the broker when memory allocation fails instead of trying to continue operations. We do have this setting in Lines 299 to 302 in 7a3ec43 There's a lot of discussion on #25021 about addressing Netty allocator problems. One of the gaps in the broker is this one: Focusing on fixing the root cause could be more effective in the long term. |
|
One source of OOM problems in dispatching might be caused by the default The way how |
|
PR to change |
Motivation
The compaction flush path leaks buffers and permits on failure:
StrategicTwoPhaseCompactor.flushBatchMessage(): the serialized batch buffer is owned solely by the local variable oncetoByteBuf()returned (the container has already cleared itself), but a failure before the BookKeeper hand-off completes — an interruptedoutstanding.acquire(), a throwing stats update, orasyncAddEntryfailing synchronously — only discarded the (already empty) container: the serialized buffer leaked on every failed flush.acquire().RawBatchMessageContainerImpl.toByteBuf(): the output buffer is allocated before the header writes, but a failure between the allocation and the return left it orphaned — the finally released the intermediate buffers only.Modifications
flushBatchMessage()releases the serialized buffer when the hand-off never happened (it is nulled after the successful hand-off, when BookKeeper owns it) and returns the permit when it was acquired — anacquire()that itself throws must not release one it never took, or the outstanding-writes limit silently grows.asyncAddEntry, and an exception escaping it would reach the caller's catch, releasing the permit and the buffer a second time after the write already took them. With the callback unable to throw, an exception fromasyncAddEntryitself can only mean the callback never ran — verified against BookKeeper 4.18.0, whose synchronous throws indoAsyncAddEntryall precede any callback execution — which is exactly the case the caller's catch handles.toByteBuf()releases the output buffer in its finally as well, nulled on the success path where its ownership moved to the returned buffer; its allocation goes through the instance allocator (the default constructor already usesPulsarByteBufAllocator.DEFAULT), which is what lets the regression test track it.Verifying this change
This change added tests and can be verified as follows:
RawBatchMessageContainerImplTest#testToByteBufReleasesTheOutputBufferWhenSerializationFails(fails with "expected [0] but found [1]" on the previous code). The compactor fixes have no direct test seam (private flush method, inline container); the reasoning is stated above.Does this pull request potentially affect one of the following parts:
Documentation
no-docneeded (internal leak fixes on the compaction flush path, no user-facing behavior or configuration change)