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/02/18 10:23:49 UTC
[GitHub] [pulsar] swjz opened a new pull request #14375: [Issue 14372][pulsar-client] Fix segfault caused by erased iterators
swjz opened a new pull request #14375:
URL: https://github.com/apache/pulsar/pull/14375
Fixes #14372
### Motivation
As described in #14372, `acknowledge_cumulative()` does not work with a consumer subscribed to multiple topics. Upon initial inspection, the segfault is caused by iterators being erased within a for loop.
### Modifications
I removed the code erasing every single consumer iterator in `MultiTopicsConsumerImpl::handleSingleConsumerClose()`. Instead, I cleared the map entirely after the for loop is done in `MultiTopicsConsumerImpl::closeAsync()`.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
This change added tests and can be verified as follows:
- testMultiTopicsConsumerAcknowledgeCumulative
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (no)
- The public API: (no)
- The schema: (no)
- The default values of configurations: (no)
- The wire protocol: (no)
- The rest endpoints: (no)
- The admin cli options: (no)
- Anything that affects deployment: (no)
### Documentation
Check the box below or label this PR directly (if you have committer privilege).
Need to update docs?
- [ ] `doc-required`
(If you need help on updating docs, create a doc issue)
- [x] `no-need-doc`
Bug fix only
- [ ] `doc`
(If this PR contains doc changes)
--
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 change in pull request #14375: [Issue 14372][C++] Fix segfault caused by erased iterators
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #14375:
URL: https://github.com/apache/pulsar/pull/14375#discussion_r809941085
##########
File path: pulsar-client-cpp/lib/MultiTopicsConsumerImpl.cc
##########
@@ -383,18 +383,14 @@ void MultiTopicsConsumerImpl::closeAsync(ResultCallback callback) {
shared_from_this(), std::placeholders::_1, topicPartitionName,
callback));
}
+ consumers_.clear();
Review comment:
I think the root cause is that the access to `consumers_` is not protected by a mutex.
The simple solution is add another `std::mutex` field to `MultiTopicsConsumerImpl`, and protect each access to `consumers_` field.
--
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] swjz closed pull request #14375: [Issue 14372][C++] Fix segfault caused by erased iterators
Posted by GitBox <gi...@apache.org>.
swjz closed pull request #14375:
URL: https://github.com/apache/pulsar/pull/14375
--
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 change in pull request #14375: [Issue 14372][C++] Fix segfault caused by erased iterators
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #14375:
URL: https://github.com/apache/pulsar/pull/14375#discussion_r809943613
##########
File path: pulsar-client-cpp/tests/BasicEndToEndTest.cc
##########
@@ -1892,6 +1892,37 @@ TEST(BasicEndToEndTest, testMultiTopicsConsumerConnectError) {
client.shutdown();
}
+TEST(BasicEndToEndTest, testMultiTopicsConsumerAcknowledgeCumulative) {
Review comment:
As I've explained in https://github.com/apache/pulsar/issues/14372#issuecomment-1044415183. The segfault is not related to `acknowledgeCumulative`.
I applied your patch and run the test in my local env, it failed as I've expected.
```
$ ./tests/main --gtest_filter='Basic*MultiTopics*Cumulative'
Note: Google Test filter = Basic*MultiTopics*Cumulative
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from BasicEndToEndTest
[ RUN ] BasicEndToEndTest.testMultiTopicsConsumerAcknowledgeCumulative
2022-02-18 20:10:28.951 INFO [0x107ddd600] ClientConnection:182 | [<none> -> pulsar://localhost:6650] Create ClientConnection, timeout=10000
2022-02-18 20:10:28.951 INFO [0x107ddd600] ConnectionPool:96 | Created connection for pulsar://localhost:6650
2022-02-18 20:10:28.952 INFO [0x700000a8d000] ClientConnection:368 | [127.0.0.1:51817 -> 127.0.0.1:6650] Connected to broker
2022-02-18 20:10:28.959 INFO [0x700000a8d000] HandlerBase:64 | [persistent://public/default/testMultiTopicsConsumerAcknowledgeCumulative-1, ] Getting connection from pool
2022-02-18 20:10:29.000 INFO [0x700000a8d000] ProducerImpl:189 | [persistent://public/default/testMultiTopicsConsumerAcknowledgeCumulative-1, ] Created producer on broker [127.0.0.1:51817 -> 127.0.0.1:6650]
2022-02-18 20:10:29.002 INFO [0x700000a8d000] HandlerBase:64 | [persistent://public/default/testMultiTopicsConsumerAcknowledgeCumulative-1, my-sub-name, 0] Getting connection from pool
2022-02-18 20:10:29.004 INFO [0x700000a8d000] HandlerBase:64 | [persistent://public/default/testMultiTopicsConsumerAcknowledgeCumulative-2, my-sub-name, 1] Getting connection from pool
2022-02-18 20:10:29.021 INFO [0x700000a8d000] ConsumerImpl:224 | [persistent://public/default/testMultiTopicsConsumerAcknowledgeCumulative-1, my-sub-name, 0] Created consumer on broker [127.0.0.1:51817 -> 127.0.0.1:6650]
2022-02-18 20:10:29.056 INFO [0x700000a8d000] ConsumerImpl:224 | [persistent://public/default/testMultiTopicsConsumerAcknowledgeCumulative-2, my-sub-name, 1] Created consumer on broker [127.0.0.1:51817 -> 127.0.0.1:6650]
2022-02-18 20:10:29.056 INFO [0x700000a8d000] MultiTopicsConsumerImpl:95 | Successfully Subscribed to Topics
pulsar-client-cpp/tests/BasicEndToEndTest.cc:1918: Failure
Expected equality of these values:
ResultOk
Which is: Ok
consumer.acknowledgeCumulative(msgReceived)
Which is: OperationNotSupported
[ FAILED ] BasicEndToEndTest.testMultiTopicsConsumerAcknowledgeCumulative (110 ms)
[----------] 1 test from BasicEndToEndTest (110 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (110 ms total)
[ PASSED ] 0 tests.
[ FAILED ] 1 test, listed below:
[ FAILED ] BasicEndToEndTest.testMultiTopicsConsumerAcknowledgeCumulative
```
--
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 #14375: [Issue 14372][C++] Fix segfault caused by erased iterators
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #14375:
URL: https://github.com/apache/pulsar/pull/14375#issuecomment-1044954067
I've opened a PR to fix the unsafe access of `consumers_`: https://github.com/apache/pulsar/pull/14380.
--
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