Skip to content

[fix] [broker]Consumption stuck due to orphan consumers after the connection reconnect - #20573

Closed
poorbarcode wants to merge 2 commits into
apache:masterfrom
poorbarcode:fix/orphan_consumer_after_recconnect
Closed

[fix] [broker]Consumption stuck due to orphan consumers after the connection reconnect#20573
poorbarcode wants to merge 2 commits into
apache:masterfrom
poorbarcode:fix/orphan_consumer_after_recconnect

Conversation

@poorbarcode

@poorbarcode poorbarcode commented Jun 13, 2023

Copy link
Copy Markdown
Contributor

Motivation

There may leave an orphan consumer after the ServerCnx closed, which would cause consumption to get stuck. For example:

time add a new consumer connection health check
1 Health check is failed
2 Push the task ServerCnx.close into the eventLoop
3 Push a task typed cmd subscribe into the eventLoop
4 connection inactive
5 Close all consumers bound to this connection
6 Register new consumers into the subscription
7 send messages to this consumer(the send will fail, but no warn log[1])
8 These messages can not be delivered until the topic is unloaded

[1]: see [improve] [broker] Improve dispatcher log to trace io error that may cause consumption stuck #20568

Modifications

Just like producer registration[2]: add a checker connection.isAtive after the consumer created complete.

[2]: https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java#LL1601C45-L1601C45

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: x

@codelipenghui codelipenghui left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice catch!

Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java Outdated
…erverCnx.java

Co-authored-by: Penghui Li <penghui@apache.org>

@merlimat merlimat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 Nice catch and the fix looks good.

I wonder if we should force-close the connection and clear the consumer as well, when there is the socket write failure because the connection was already closed.

@poorbarcode

Copy link
Copy Markdown
Contributor Author

@poorbarcode

Copy link
Copy Markdown
Contributor Author

I wonder if we should force-close the connection and clear the consumer as well, when there is the socket write failure because the connection was already closed.

I did it in the PR #20568: do a health check immediately(if the health check fail, the connection will be closed) if got an IO error when sending messages to the client.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants