Skip to content

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

Merged
BewareMyPower merged 2 commits into
apache:mainfrom
erobot:fix-close-producer
Dec 1, 2022
Merged

[fix] Close broker producer created after producer close#131
BewareMyPower merged 2 commits into
apache:mainfrom
erobot:fix-close-producer

Conversation

@erobot
Copy link
Copy Markdown
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
Comment thread lib/ProducerImpl.cc
Comment thread lib/ProducerImpl.cc Outdated
Comment thread lib/ProducerImpl.cc
@BewareMyPower
Copy link
Copy Markdown
Contributor

Please rebase to master to avoid broken CI

@erobot erobot force-pushed the fix-close-producer branch from 75ef146 to b0ae51d Compare December 1, 2022 08:01
@erobot
Copy link
Copy Markdown
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