Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Broker] Fix race conditions in closing producers and consumers #13428

Merged

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Dec 21, 2021

Motivation

  • closing ServerCnx while producers or consumers are created can lead to a producer or consumer never getting removed from the topic's list of producers

Modifications

If the future isn't completed by the time of closing the ServerCnx, complete it exceptionally. This will ensure that the existing logic in creating a producer or consumer doesn't leave the just created producer or consumer open.

- closing ServerCnx while producers or consumers are created can lead
  to a producer or consumer never getting removed from the topic's
  list of producers
@lhotari lhotari added this to the 2.10.0 milestone Dec 21, 2021
@lhotari lhotari self-assigned this Dec 21, 2021
lhotari added a commit to datastax/pulsar that referenced this pull request Dec 21, 2021
…he#13428)

- closing ServerCnx while producers or consumers are created can lead
  to a producer or consumer never getting removed from the topic's
  list of producers

upstream pull request apache#13428
@lhotari lhotari requested a review from ivankelly December 21, 2021 09:56
Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

LGTM, other than a couple nits.

It's worth pointing out that the race is very subtle and comes here:

if (isActive()) {
if (producerFuture.complete(producer)) {
log.info("[{}] Created new producer: {}", remoteAddress, producer);
commandSender.sendProducerSuccessResponse(requestId, producerName,
producer.getLastSequenceId(), producer.getSchemaVersion(),
newTopicEpoch, true /* producer is ready now */);
if (getBrokerService().getInterceptor() != null) {
getBrokerService().getInterceptor().
producerCreated(this, producer, metadata);
}
return;
} else {
// The producer's future was completed before by
// a close command
producer.closeNow(true);
log.info("[{}] Cleared producer created after"
+ " timeout on client side {}",
remoteAddress, producer);
}
} else {
producer.closeNow(true);
log.info("[{}] Cleared producer created after connection was closed: {}",
remoteAddress, producer);
producerFuture.completeExceptionally(
new IllegalStateException(
"Producer created after connection was closed"));
}

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

👍

@lhotari lhotari merged commit 3316db5 into apache:master Dec 22, 2021
codelipenghui pushed a commit that referenced this pull request Dec 22, 2021
- closing ServerCnx while producers or consumers are created can lead
  to a producer or consumer never getting removed from the topic's
  list of producers

(cherry picked from commit 3316db5)
@codelipenghui codelipenghui added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Dec 22, 2021
zymap pushed a commit that referenced this pull request Dec 23, 2021
- closing ServerCnx while producers or consumers are created can lead
  to a producer or consumer never getting removed from the topic's
  list of producers

(cherry picked from commit 3316db5)
@zymap zymap added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Dec 23, 2021
Technoboy- pushed a commit to Technoboy-/pulsar that referenced this pull request Dec 29, 2021
…he#13428)

- closing ServerCnx while producers or consumers are created can lead
  to a producer or consumer never getting removed from the topic's
  list of producers
aloyszhang pushed a commit to aloyszhang/pulsar that referenced this pull request Jan 9, 2023
Squash merge branch '2.8.1.4_LeastLongTermMessageRate' into 'release-2.8.1.4'
Fixes #<xyz>

### Motivation
--bug=101470445 Ledger元数据信息不一致和数据丢失问题 (merge request !90) 
--bug=105950701 【Pulsar】腾讯视频consumer泄露问题定位和修复  apache#13428 (merge request !163)
--bug=106044205 Broker启动时,LeastLongTermMessageRate加载失败问题修复 apache#16096 (merge request !162) 

TAPD: --bug=106044205
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life doc-not-needed Your PR changes do not impact docs release/2.8.3 release/2.9.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants