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/03/23 17:50:48 UTC
[GitHub] [pulsar] BewareMyPower opened a new pull request #14823: [C++] Fix the race condition of connect timeout task
BewareMyPower opened a new pull request #14823:
URL: https://github.com/apache/pulsar/pull/14823
Fixes #14665
### Motivation
In C++ client, a connect timeout task is created each time before an
asynchronous connect operation is performed, if the connection cannot be
established in the configured timeout, the callback of the task will be
called to close the connection and then the `createProducer` or
`subscribe` methods will return `ResultConnectError`.
`ClientConnection::connectTimeoutTask_`, which is a shared pointer,
represents the timeout task. However, after `ClientConnection::close` is
called, the shared pointer will be reset, and the underlying `PeriodicTask`
object will be released. After that, when `stop` method is called on the
released `PeriodicTask` object in the callback (`handleTcpConnected`), a
segmentation fault will happen.
The root cause is that `connectTimeoutTask_` can be accessed in two
threads while one of them could release the memory. See #14665 for more
explanations. This race condition leads to flaky Python tests as well,
because we also have the similar test in Python tests. See
https://github.com/apache/pulsar/blob/f7cbc1eb83ffd27b784d90d5d2dea8660c590ad2/pulsar-client-cpp/python/pulsar_test.py#L1207-L1221
So this PR might also fix #14714.
### Modifications
Remove `connectTimeoutTask_.reset()` in `ClientConnection::close`. After
that, the `connectTimeoutTask_` will always points to the same
`PeriodicTask` object, whose methods are thread safe.
### Verifying this change
Execute the following command
```bash
./tests/main --gtest_filter='ClientTest.testConnectTimeout' --gtest_repeat=10
```
to runs the `testConnectTimeout` for 10 times. In my local env, it never
failed, while before applying this patch, it's very easy to fail.
--
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 #14823: [C++] Fix the race condition of connect timeout task
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #14823:
URL: https://github.com/apache/pulsar/pull/14823#issuecomment-1077029691
I just found this PR brings a regression of https://github.com/apache/pulsar/pull/14587 and I've opened another PR (https://github.com/apache/pulsar/pull/14834) to fix it.
--
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] lhotari merged pull request #14823: [C++] Fix the race condition of connect timeout task
Posted by GitBox <gi...@apache.org>.
lhotari merged pull request #14823:
URL: https://github.com/apache/pulsar/pull/14823
--
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 #14823: [C++] Fix the race condition of connect timeout task
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #14823:
URL: https://github.com/apache/pulsar/pull/14823#issuecomment-1077013835
This bug was introduced from https://github.com/apache/pulsar/pull/14587, which was not cherry-picked into branch-2.8 and branch-2.9. I'll cherry-pick it first.
I found the flaky test on branch-2.9 is `testReferenceCount` (https://github.com/apache/pulsar/issues/14719), I'll work on this issue soon.
```
FAILED TESTS (2/274):
37 ms: ./main ClientTest.testReferenceCount (try #1)
31 ms: ./main ClientTest.testReferenceCount (try #2)
```
--
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