Skip to content

Fix multi-topics consumer will crash if one internal consumer fails getBrokerConsumerStatsAsync#538

Open
BewareMyPower wants to merge 3 commits intoapache:mainfrom
BewareMyPower:bewaremypower/fix-multi-consumer-get-stats-crash
Open

Fix multi-topics consumer will crash if one internal consumer fails getBrokerConsumerStatsAsync#538
BewareMyPower wants to merge 3 commits intoapache:mainfrom
BewareMyPower:bewaremypower/fix-multi-consumer-get-stats-crash

Conversation

@BewareMyPower
Copy link
Contributor

Fixes #536

The root cause is the when one internal consumer fails, the callback will be triggered immediately before other internal consumers finish the RPC. Then the getBrokerConsumerStats will return and the Promise object will be destroyed.

When another consumer's callback is called, it will try to complete the destroyed Promise object referred by the callback wrapper (WaitForCallbackValue), which leads to the crash.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a critical crash in multi-topic consumer when one internal consumer fails during getBrokerConsumerStatsAsync. The root cause was that when one consumer failed, the callback was invoked before other consumers completed, causing the Promise object to be destroyed. When other callbacks tried to complete the destroyed Promise, it led to a crash.

Changes:

  • Replaced Latch with std::atomic_size_t for countdown synchronization to prevent premature Promise destruction
  • Added atomic tracking of failed results to ensure all callbacks complete before invoking the final callback
  • Added test coverage for the failure scenario with MockServer support
  • Fixed missing return statement in ClientConnection::newConsumerStats when connection is closed

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
lib/MultiTopicsConsumerImpl.cc Refactored getBrokerConsumerStatsAsync to use atomic counters and ensure all callbacks complete before returning; removed handleGetConsumerStats method
lib/MultiTopicsConsumerImpl.h Removed Latch.h include and handleGetConsumerStats method declaration
lib/ClientConnection.cc Added missing return statement after setting promise to failed when connection is closed; added mock server support for CONSUMER_STATS requests
lib/ClientConnection.h Made handleKeepAliveTimeout public for test access
lib/MockServer.h Added proper handling of CONSUMER_STATS requests to return CommandConsumerStatsResponse instead of generic success
tests/MultiTopicsConsumerTest.cc Added test case to verify correct behavior when consumer stats request fails due to connection timeout
tests/ConsumerTest.cc Added explicit Latch.h include (previously transitive through MultiTopicsConsumerImpl.h)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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.

[Bug] Crash in getBrokerConsumerStatsAsync when connection is closed

1 participant