You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/08/23 13:41:04 UTC
[GitHub] [pulsar] BewareMyPower opened a new pull request, #17239: [fix][cpp] Fix multi-topics consumer close segmentation fault
BewareMyPower opened a new pull request, #17239:
URL: https://github.com/apache/pulsar/pull/17239
### Motivation
When a multi-topics consumer fails to create, it will close all internal
single consumers. In this case, segmentation fault will happen.
https://github.com/apache/pulsar/blob/8286e8ccb2e1433814c306607c6f218ca548752a/pulsar-client-cpp/lib/MultiTopicsConsumerImpl.cc#L369-L374
https://github.com/apache/pulsar/blob/8286e8ccb2e1433814c306607c6f218ca548752a/pulsar-client-cpp/lib/ConsumerImpl.cc#L984-L989
If the single consumer failed to create, the callback here
(`handleSingleConsumerClose`) will be called immediately in the range
for loop of `consumers_`. However, the callback also modified the
`consumers_` by calling `remove` and `clear` methods.
### Modifications
- Add a new overload for `SynchronizedHashMap::clear` that accepts a
callback on the key-value. This `clear` overload calls the
callback on each key-value pair before removing it. Use this `clear`
overload instead of `forEach` in `MultiTopicsConsumerImpl::closeAsync`.
- Add an atomic result field `failedResult` to save the 1st error code
when creating a multi-topics consumer. Complete the
`multiTopicsConsumerCreatedPromise_` with this cached result in the
callback of `closeAsync`.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
- Modify `SynchronizedHashMap::testClear` to verify the new overload of
`clear` method.
- Modify `ClientTest::testWrongListener` to cover the case when a
multi-topics consumer is closed by a failed creation.
### Documentation
Check the box below or label this PR directly.
Need to update docs?
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- merged pull request #17239: [fix][cpp] Fix multi-topics consumer close segmentation fault
Posted by GitBox <gi...@apache.org>.
Technoboy- merged PR #17239:
URL: https://github.com/apache/pulsar/pull/17239
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #17239: [fix][cpp] Fix multi-topics consumer close segmentation fault
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #17239:
URL: https://github.com/apache/pulsar/pull/17239#discussion_r953262625
##########
pulsar-client-cpp/lib/MultiTopicsConsumerImpl.cc:
##########
@@ -364,13 +366,47 @@ void MultiTopicsConsumerImpl::closeAsync(ResultCallback callback) {
state_ = Closing;
- auto self = shared_from_this();
+ std::weak_ptr<MultiTopicsConsumerImpl> weakSelf{shared_from_this()};
Review Comment:
To avoid extending the lifetime of `MultiTopicsConsumerImpl`. e.g.
```c++
{
consumer.closeAsync(nullptr);
} // consumer is destroyed here
```
If we capture a shared pointer here in handleOneTopicSubscribed, the callback will still be executed after the consumer is destroyed.
Actually, the best practice for `shared_ptr` and `weak_ptr` is to specify the owner and only the owner holds a `shared_ptr`. If the owner wants to share it to other objects, it should pass a `std::weak_ptr`. This best practice could avoid cyclic dependency. (I might push another PR to follow this rule)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] shibd commented on a diff in pull request #17239: [fix][cpp] Fix multi-topics consumer close segmentation fault
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #17239:
URL: https://github.com/apache/pulsar/pull/17239#discussion_r952805482
##########
pulsar-client-cpp/lib/MultiTopicsConsumerImpl.cc:
##########
@@ -86,11 +86,14 @@ void MultiTopicsConsumerImpl::handleOneTopicSubscribed(Result result, Consumer c
if (result != ResultOk) {
state_ = Failed;
+ // Use the first failed result
+ auto expectedResult = ResultOk;
+ failedResult.compare_exchange_strong(expectedResult, result);
Review Comment:
Why is `failedResult` a member variable, can you directly set the `result` to line 107? I haven't seen `failedResult` used elsewhere.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] shibd commented on a diff in pull request #17239: [fix][cpp] Fix multi-topics consumer close segmentation fault
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #17239:
URL: https://github.com/apache/pulsar/pull/17239#discussion_r952793882
##########
pulsar-client-cpp/lib/MultiTopicsConsumerImpl.cc:
##########
@@ -364,13 +366,47 @@ void MultiTopicsConsumerImpl::closeAsync(ResultCallback callback) {
state_ = Closing;
- auto self = shared_from_this();
+ std::weak_ptr<MultiTopicsConsumerImpl> weakSelf{shared_from_this()};
Review Comment:
Why here use weak ptr?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] shibd commented on a diff in pull request #17239: [fix][cpp] Fix multi-topics consumer close segmentation fault
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #17239:
URL: https://github.com/apache/pulsar/pull/17239#discussion_r953283238
##########
pulsar-client-cpp/lib/MultiTopicsConsumerImpl.cc:
##########
@@ -364,13 +366,47 @@ void MultiTopicsConsumerImpl::closeAsync(ResultCallback callback) {
state_ = Closing;
- auto self = shared_from_this();
+ std::weak_ptr<MultiTopicsConsumerImpl> weakSelf{shared_from_this()};
Review Comment:
Got it. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #17239: [fix][cpp] Fix multi-topics consumer close segmentation fault
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #17239:
URL: https://github.com/apache/pulsar/pull/17239#issuecomment-1227054415
Hi @BewareMyPower
Would you like to cherry-pick this PR for branch-2.9 to fix this problem?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #17239: [fix][cpp] Fix multi-topics consumer close segmentation fault
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #17239:
URL: https://github.com/apache/pulsar/pull/17239#issuecomment-1227338032
@mattisonchao Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] shibd commented on a diff in pull request #17239: [fix][cpp] Fix multi-topics consumer close segmentation fault
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #17239:
URL: https://github.com/apache/pulsar/pull/17239#discussion_r953283238
##########
pulsar-client-cpp/lib/MultiTopicsConsumerImpl.cc:
##########
@@ -364,13 +366,47 @@ void MultiTopicsConsumerImpl::closeAsync(ResultCallback callback) {
state_ = Closing;
- auto self = shared_from_this();
+ std::weak_ptr<MultiTopicsConsumerImpl> weakSelf{shared_from_this()};
Review Comment:
Get it. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #17239: [fix][cpp] Fix multi-topics consumer close segmentation fault
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #17239:
URL: https://github.com/apache/pulsar/pull/17239#discussion_r953255225
##########
pulsar-client-cpp/lib/MultiTopicsConsumerImpl.cc:
##########
@@ -86,11 +86,14 @@ void MultiTopicsConsumerImpl::handleOneTopicSubscribed(Result result, Consumer c
if (result != ResultOk) {
state_ = Failed;
+ // Use the first failed result
+ auto expectedResult = ResultOk;
+ failedResult.compare_exchange_strong(expectedResult, result);
Review Comment:
No. line 107 is in the block of `if (topicsNeedCreate->load() == 0)`, which means only the last time `handleOneTopicSubscribed` is called. Usually, when multiple topics failed to subscribe, the 1st topic would fail with the real reason, but the following topics would fail with `ResultAlreadyClosed`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] shibd commented on a diff in pull request #17239: [fix][cpp] Fix multi-topics consumer close segmentation fault
Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #17239:
URL: https://github.com/apache/pulsar/pull/17239#discussion_r953272208
##########
pulsar-client-cpp/lib/MultiTopicsConsumerImpl.cc:
##########
@@ -86,11 +86,14 @@ void MultiTopicsConsumerImpl::handleOneTopicSubscribed(Result result, Consumer c
if (result != ResultOk) {
state_ = Failed;
+ // Use the first failed result
+ auto expectedResult = ResultOk;
+ failedResult.compare_exchange_strong(expectedResult, result);
Review Comment:
Get it, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org