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 2020/02/10 22:49:04 UTC
[GitHub] [pulsar] merlimat opened a new pull request #6287: [C++] Fixed
triggering timers when the connection is being closed
merlimat opened a new pull request #6287: [C++] Fixed triggering timers when the connection is being closed
URL: https://github.com/apache/pulsar/pull/6287
### Motivation
Some of the C++ unit testings are failing with segfaults on the CI jobs. This happens when running on containers with low resource. This happens in particular in tests that are very quick to execute and tear down, such as `BasicEndToEndTest.testLookupThrottling` or `BasicEndToEndTest.testPatternEmptyUnsubscribe`.
Example of core dump stack traces:
```
Thread 1 (Thread 0x7fc838357700 (LWP 23473)):
#0 0x00007fc83b9daf1e in boost::asio::basic_io_object<boost::asio::detail::deadline_timer_service<boost::asio::time_traits<boost::posix_time::ptime> >, true>::get_service (this=0x0) at /usr/include/boost/asio/basic_io_object.hpp:260
#1 0x00007fc83ba1f80f in boost::asio::basic_deadline_timer<boost::posix_time::ptime, boost::asio::time_traits<boost::posix_time::ptime> >::async_wait<std::_Bind<void (pulsar::ClientConnection::*(std::shared_ptr<pulsar::ClientConnection>))()> >(std::_Bind<void (pulsar::ClientConnection::*(std::shared_ptr<pulsar::ClientConnection>))()>&&) (this=0x0, handler=...) at /usr/include/boost/asio/basic_deadline_timer.hpp:610
#2 0x00007fc83ba047b2 in pulsar::ClientConnection::handlePulsarConnected (this=0x556d945962a0, cmdConnected=...) at /pulsar/pulsar-client-cpp/lib/ClientConnection.cc:258
#3 0x00007fc83ba08d55 in pulsar::ClientConnection::handleIncomingCommand (this=0x556d945962a0) at /pulsar/pulsar-client-cpp/lib/ClientConnection.cc:670
#4 0x00007fc83ba07dcd in pulsar::ClientConnection::processIncomingBuffer (this=0x556d945962a0) at /pulsar/pulsar-client-cpp/lib/ClientConnection.cc:574
#5 0x00007fc83ba074c5 in pulsar::ClientConnection::handleRead (this=0x556d945962a0, err=..., bytesTransferred=34, minReadSize=4) at /pulsar/pulsar-client-cpp/lib/ClientConnection.cc:498
#6 0x00007fc83ba5ccbb in std::__invoke_impl<void, void (pulsar::ClientConnection::*&)(boost::system::error_code const&, unsigned long, unsigned int), std::shared_ptr<pulsar::ClientConnection>&, boost::system::error_code&, unsigned long&, unsigned in
```
and
```
#0 0x00007fea2bb7f00e in boost::asio::basic_io_object<boost::asio::detail::deadline_timer_service<boost::asio::time_traits<boost::posix_time::ptime> >, true>::get_service (this=0x0) at /usr/include/boost/asio/basic_io_object.hpp:260
#1 0x00007fea2bb7e09a in boost::asio::basic_deadline_timer<boost::posix_time::ptime, boost::asio::time_traits<boost::posix_time::ptime> >::expires_from_now (this=0x0, expiry_time=...) at /usr/include/boost/asio/basic_deadline_timer.hpp:509
#2 0x00007fea2bba8ead in pulsar::ClientConnection::startConsumerStatsTimer (this=0x55f8e18382a0, consumerStatsRequests=std::vector of length 0, capacity 0) at /pulsar/pulsar-client-cpp/lib/ClientConnection.cc:287
#3 0x00007fea2bba891c in pulsar::ClientConnection::handlePulsarConnected (this=0x55f8e18382a0, cmdConnected=...) at /pulsar/pulsar-client-cpp/lib/ClientConnection.cc:262
#4 0x00007fea2bbace45 in pulsar::ClientConnection::handleIncomingCommand (this=0x55f8e18382a0) at /pulsar/pulsar-client-cpp/lib/ClientConnection.cc:670
#5 0x00007fea2bbabebd in pulsar::ClientConnection::processIncomingBuffer (this=0x55f8e18382a0) at /pulsar/pulsar-client-cpp/lib/ClientConnection.cc:574
#6 0x00007fea2bbab5b5 in pulsar::ClientConnection::handleRead (this=0x55f8e18382a0, err=..., bytesTransferred=34, minReadSize=4) at /pulsar/pulsar-client-cpp/lib/ClientConnection.cc:498
```
In both cases, what happens is that on one side we're still initializing the connection timers, while on the other we're closing down the connection (eg: on `Client.close()`).
### Modifications
Make sure we're not trying to use a shared pointer which is already reset by a different thread.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] merlimat merged pull request #6287: [C++] Fixed triggering
timers when the connection is being closed
Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #6287: [C++] Fixed triggering timers when the connection is being closed
URL: https://github.com/apache/pulsar/pull/6287
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] jai1 commented on a change in pull request #6287: [C++]
Fixed triggering timers when the connection is being closed
Posted by GitBox <gi...@apache.org>.
jai1 commented on a change in pull request #6287: [C++] Fixed triggering timers when the connection is being closed
URL: https://github.com/apache/pulsar/pull/6287#discussion_r377399594
##########
File path: pulsar-client-cpp/lib/ClientConnection.cc
##########
@@ -253,9 +253,10 @@ void ClientConnection::handlePulsarConnected(const CommandConnected& cmdConnecte
if (serverProtocolVersion_ >= v1) {
// Only send keep-alive probes if the broker supports it
- keepAliveTimer_ = executor_->createDeadlineTimer();
- keepAliveTimer_->expires_from_now(boost::posix_time::seconds(KeepAliveIntervalInSeconds));
- keepAliveTimer_->async_wait(std::bind(&ClientConnection::handleKeepAliveTimeout, shared_from_this()));
+ DeadlineTimerPtr keepAliveTimer = executor_->createDeadlineTimer();
Review comment:
nit: Can you create a separate function for this?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] merlimat commented on a change in pull request #6287:
[C++] Fixed triggering timers when the connection is being closed
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #6287: [C++] Fixed triggering timers when the connection is being closed
URL: https://github.com/apache/pulsar/pull/6287#discussion_r377428428
##########
File path: pulsar-client-cpp/lib/ClientConnection.cc
##########
@@ -284,10 +285,14 @@ void ClientConnection::startConsumerStatsTimer(std::vector<uint64_t> consumerSta
it != pendingConsumerStatsMap_.end(); ++it) {
consumerStatsRequests.push_back(it->first);
}
- consumerStatsRequestTimer_->expires_from_now(operationsTimeout_);
- consumerStatsRequestTimer_->async_wait(std::bind(&ClientConnection::handleConsumerStatsTimeout,
- shared_from_this(), std::placeholders::_1,
- consumerStatsRequests));
+
+ DeadlineTimerPtr timer = consumerStatsRequestTimer_;
+ if (timer) {
+ timer->expires_from_now(operationsTimeout_);
+ timer->async_wait(std::bind(&ClientConnection::handleConsumerStatsTimeout, shared_from_this(),
+ std::placeholders::_1, consumerStatsRequests));
+ }
Review comment:
No, it's already sharing a reference to the same object. The local copy is just to keep it alive while we're using 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] jai1 commented on a change in pull request #6287: [C++]
Fixed triggering timers when the connection is being closed
Posted by GitBox <gi...@apache.org>.
jai1 commented on a change in pull request #6287: [C++] Fixed triggering timers when the connection is being closed
URL: https://github.com/apache/pulsar/pull/6287#discussion_r377399362
##########
File path: pulsar-client-cpp/lib/ClientConnection.cc
##########
@@ -284,10 +285,14 @@ void ClientConnection::startConsumerStatsTimer(std::vector<uint64_t> consumerSta
it != pendingConsumerStatsMap_.end(); ++it) {
consumerStatsRequests.push_back(it->first);
}
- consumerStatsRequestTimer_->expires_from_now(operationsTimeout_);
- consumerStatsRequestTimer_->async_wait(std::bind(&ClientConnection::handleConsumerStatsTimeout,
- shared_from_this(), std::placeholders::_1,
- consumerStatsRequests));
+
+ DeadlineTimerPtr timer = consumerStatsRequestTimer_;
Review comment:
Not sure if we need this, since the operations are done under a lock.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] jai1 commented on a change in pull request #6287: [C++]
Fixed triggering timers when the connection is being closed
Posted by GitBox <gi...@apache.org>.
jai1 commented on a change in pull request #6287: [C++] Fixed triggering timers when the connection is being closed
URL: https://github.com/apache/pulsar/pull/6287#discussion_r377399506
##########
File path: pulsar-client-cpp/lib/ClientConnection.cc
##########
@@ -284,10 +285,14 @@ void ClientConnection::startConsumerStatsTimer(std::vector<uint64_t> consumerSta
it != pendingConsumerStatsMap_.end(); ++it) {
consumerStatsRequests.push_back(it->first);
}
- consumerStatsRequestTimer_->expires_from_now(operationsTimeout_);
- consumerStatsRequestTimer_->async_wait(std::bind(&ClientConnection::handleConsumerStatsTimeout,
- shared_from_this(), std::placeholders::_1,
- consumerStatsRequests));
+
+ DeadlineTimerPtr timer = consumerStatsRequestTimer_;
+ if (timer) {
+ timer->expires_from_now(operationsTimeout_);
+ timer->async_wait(std::bind(&ClientConnection::handleConsumerStatsTimeout, shared_from_this(),
+ std::placeholders::_1, consumerStatsRequests));
+ }
Review comment:
Don't we need
`consumerStatsRequestTimer_ = timer` here?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] merlimat commented on a change in pull request #6287:
[C++] Fixed triggering timers when the connection is being closed
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #6287: [C++] Fixed triggering timers when the connection is being closed
URL: https://github.com/apache/pulsar/pull/6287#discussion_r377428197
##########
File path: pulsar-client-cpp/lib/ClientConnection.cc
##########
@@ -284,10 +285,14 @@ void ClientConnection::startConsumerStatsTimer(std::vector<uint64_t> consumerSta
it != pendingConsumerStatsMap_.end(); ++it) {
consumerStatsRequests.push_back(it->first);
}
- consumerStatsRequestTimer_->expires_from_now(operationsTimeout_);
- consumerStatsRequestTimer_->async_wait(std::bind(&ClientConnection::handleConsumerStatsTimeout,
- shared_from_this(), std::placeholders::_1,
- consumerStatsRequests));
+
+ DeadlineTimerPtr timer = consumerStatsRequestTimer_;
Review comment:
The timer is not destroyed under a lock
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services