FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[fix][client] Prevent duplicate cleanup and leaks on producer send failures by Denovo1998 · Pull Request #26641 · apache/pulsar · GitHub

/ pulsar Public

[fix][client] Prevent duplicate cleanup and leaks on producer send failures - #26641

Open
Denovo1998 wants to merge 3 commits into
apache:masterfrom
Denovo1998:fix-producer-send-failure-cleanup
Open

Denovo1998 wants to merge 3 commits into
apache:masterfrom
Denovo1998:fix-producer-send-failure-cleanup

Conversation

Denovo1998 commented Sep 18, 2026
edited
Loading

Copy link
Copy Markdown
Contributor

Motivation

This follows up on #26455 to address three remaining producer send-failure cases. An oversized message serialized after schema registration can be cleaned up twice if its failure callback closes the producer during reconnect. A rejected chunk write can leave the send loop building subsequent chunks after the message has already failed, causing repeated callbacks and buffer releases. Interrupting a blocking chunked send also leaves references held for chunks that were never created.

Modifications

  • Separate message-size validation from failure completion and remove failed deferred operations from the pending queue before invoking callbacks.
  • Centralize failed-operation cleanup and ensure command release and operation recycling run even if a callback throws.
  • Recheck the connection, epoch, and producer state throughout pending-message recovery, including after the last callback. Cover both size and schema failures while preserving connection cleanup on close.
  • Stop building chunks after an operation fails and release the payload, memory reservation, permits, and context references belonging to unbuilt chunks.
  • Release unbuilt chunk context references when a blocking send is interrupted, and document the locking, lifetime, and callback contracts.
  • Extend ProducerMemoryLeakTest with real broker/client paths, deterministic event-loop task rejection, thread interruption, and callback-triggered close after a broker schema rejection.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

nodece left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for the follow-up — the ownership accounting for the three failure paths checks out (I traced the payload/ctx/permit/memory balances per path, and ran ProducerMemoryLeakTest on this branch locally: 22/22 pass).

The inline comments below are structural/consistency suggestions rather than correctness issues — no blockers from my side. One non-code nit: the PR description still contains template residue (the "trivial rework / covered by existing tests / added tests" options and the example verification bullets) — worth cleaning up before merge.

Review assisted by Claude Code (AI-assisted review), posted by @nodece.

Denovo1998 requested a review from nodece September 21, 2026 11:32

Copy link
Copy Markdown
Contributor Author

@nodece Just made a fix, please review it again.

Copy link
Copy Markdown
Contributor Author

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

2 participants


Back | FazBrowse Home | New Git URL