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

[fix] Close broker producer created after producer close #131

Merged
merged 2 commits into from
Dec 1, 2022

Conversation

erobot
Copy link
Contributor

@erobot erobot commented Nov 30, 2022

Motivation

Close broker producer created after producer close to prevent producers from keeping alive in broker, and fix a race condition here.

Race condition sequence:

  1. prodcuer.start() is called
  2. handleCreateProducer() is called, and runs to right before setCnx()
  3. closeAsync() is called and returns directly after getCnx() returns nullptr, so no CloseProducer cmd is sent
  4. handleCreateProducer() continues
  5. Result: Producer is closed and no CloseProducer cmd is sent to broker

Modifications

  • Close broker producer created after producer close
  • Use mutex to prevent race condition. handleCreateProducer() and closeAsync() should be low frequency operations and using mutex here should not affect performance.

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Added unit test ProducerTest.testCloseProducerBeforeCreated

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    bug fix only

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@RobertIndie RobertIndie added this to the 3.2.0 milestone Nov 30, 2022
lib/ProducerImpl.cc Outdated Show resolved Hide resolved
@BewareMyPower
Copy link
Contributor

Please rebase to master to avoid broken CI

@erobot
Copy link
Contributor Author

erobot commented Dec 1, 2022

Rebased

@BewareMyPower BewareMyPower merged commit 34d62fa into apache:main Dec 1, 2022
@merlimat merlimat added bug Something isn't working release/3.1 labels Dec 2, 2022
merlimat pushed a commit that referenced this pull request Dec 2, 2022
### Motivation

Close broker producer created after producer close to prevent producers from keeping alive in broker, and fix a race condition here.

Race condition sequence:
1. prodcuer.start() is called
2. handleCreateProducer() is called, and runs to right before setCnx()
3. closeAsync() is called and returns directly after getCnx() returns nullptr, so no CloseProducer cmd is sent
4. handleCreateProducer() continues
5. Result: Producer is closed and no CloseProducer cmd is sent to broker

### Modifications

* Close broker producer created after producer close
* Use mutex to prevent race condition. handleCreateProducer() and closeAsync() should be low frequency operations and using mutex here should not affect performance.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants