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/07/17 05:12:00 UTC
[GitHub] [pulsar] irairdon opened a new pull request #7572: Kevin Wilson changes to fix segment crashes in pulsar
irairdon opened a new pull request #7572:
URL: https://github.com/apache/pulsar/pull/7572
----------------------------------------------------------------
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
[GitHub] [pulsar] jiazhai removed a comment on pull request #7572: [CPP] Fix segment crashes that caused by race condition of timer in cpp client
Posted by GitBox <gi...@apache.org>.
jiazhai removed a comment on pull request #7572:
URL: https://github.com/apache/pulsar/pull/7572#issuecomment-662833516
/pulsarbot run-failure-checks
----------------------------------------------------------------
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
[GitHub] [pulsar] jiazhai commented on pull request #7572: [CPP] Fix segment crashes that caused by race condition of timer in cpp client
Posted by GitBox <gi...@apache.org>.
jiazhai commented on pull request #7572:
URL: https://github.com/apache/pulsar/pull/7572#issuecomment-662871884
/pulsarbot run-failure-checks
----------------------------------------------------------------
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
[GitHub] [pulsar] jiazhai commented on pull request #7572: [CPP] Fix segment crashes that caused by race condition of timer in cpp client
Posted by GitBox <gi...@apache.org>.
jiazhai commented on pull request #7572:
URL: https://github.com/apache/pulsar/pull/7572#issuecomment-664758873
/pulsarbot run-failure-checks
----------------------------------------------------------------
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
[GitHub] [pulsar] merlimat commented on a change in pull request #7572: [CPP] Fix segment crashes that caused by race condition of timer in cpp client
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #7572:
URL: https://github.com/apache/pulsar/pull/7572#discussion_r460076343
##########
File path: pulsar-client-cpp/lib/ClientConnection.cc
##########
@@ -1363,6 +1375,8 @@ void ClientConnection::close() {
if (isClosed()) {
return;
}
+ // Use a sleep to stress the timers when the pop to find the race conditions with close().
+ // sleep(30);
Review comment:
Please remove comment
----------------------------------------------------------------
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
[GitHub] [pulsar] klwilson227 commented on pull request #7572: Kevin Wilson changes to fix segment crashes in pulsar
Posted by GitBox <gi...@apache.org>.
klwilson227 commented on pull request #7572:
URL: https://github.com/apache/pulsar/pull/7572#issuecomment-661970553
We provided the PR so that you could SEE what we found as the issue. Jai, can you clearly take it from here. As there are several other CORE issues in the code that I am attempting to identify the cause of? We can cancel this PR if needed...
----------------------------------------------------------------
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
[GitHub] [pulsar] jiazhai commented on a change in pull request #7572: Kevin Wilson changes to fix segment crashes in pulsar
Posted by GitBox <gi...@apache.org>.
jiazhai commented on a change in pull request #7572:
URL: https://github.com/apache/pulsar/pull/7572#discussion_r456792381
##########
File path: pulsar-client-cpp/lib/ClientConnection.cc
##########
@@ -117,9 +118,6 @@ static Result getResult(ServerError serverError) {
case InvalidTxnStatus:
return ResultInvalidTxnStatusError;
-
Review comment:
The changes in this file seems removed some of the latest change in master. This will cause regressions.
----------------------------------------------------------------
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
[GitHub] [pulsar] jiazhai commented on a change in pull request #7572: Kevin Wilson changes to fix segment crashes in pulsar
Posted by GitBox <gi...@apache.org>.
jiazhai commented on a change in pull request #7572:
URL: https://github.com/apache/pulsar/pull/7572#discussion_r456792240
##########
File path: pulsar-client-cpp/lib/ClientConnection.cc
##########
@@ -25,6 +25,7 @@
#include <boost/date_time/posix_time/posix_time.hpp>
#include <boost/date_time/gregorian/gregorian.hpp>
#include <climits>
+// #include <unistd.h>
Review comment:
Could we remove 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
[GitHub] [pulsar] jiazhai commented on a change in pull request #7572: Kevin Wilson changes to fix segment crashes in pulsar
Posted by GitBox <gi...@apache.org>.
jiazhai commented on a change in pull request #7572:
URL: https://github.com/apache/pulsar/pull/7572#discussion_r456792482
##########
File path: pulsar-client-cpp/lib/ClientConnection.cc
##########
@@ -1038,15 +1027,6 @@ void ClientConnection::handleIncomingCommand() {
break;
}
- case BaseCommand::AUTH_CHALLENGE: {
Review comment:
same as above comments. this change is not needed for this fix?
----------------------------------------------------------------
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
[GitHub] [pulsar] jiazhai commented on a change in pull request #7572: Kevin Wilson changes to fix segment crashes in pulsar
Posted by GitBox <gi...@apache.org>.
jiazhai commented on a change in pull request #7572:
URL: https://github.com/apache/pulsar/pull/7572#discussion_r456792460
##########
File path: pulsar-client-cpp/lib/ClientConnection.cc
##########
@@ -417,15 +415,6 @@ void ClientConnection::handleSentPulsarConnect(const boost::system::error_code&
readNextCommand();
}
-void ClientConnection::handleSentAuthResponse(const boost::system::error_code& err,
Review comment:
same as the above comments. this change is not needed for this fix?
----------------------------------------------------------------
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
[GitHub] [pulsar] jiazhai commented on pull request #7572: [CPP] Fix segment crashes that caused by race condition of timer in cpp client
Posted by GitBox <gi...@apache.org>.
jiazhai commented on pull request #7572:
URL: https://github.com/apache/pulsar/pull/7572#issuecomment-662833448
/pulsarbot run-failure-checks
----------------------------------------------------------------
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
[GitHub] [pulsar] wolfstudy commented on pull request #7572: [CPP] Fix segment crashes that caused by race condition of timer in cpp client
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on pull request #7572:
URL: https://github.com/apache/pulsar/pull/7572#issuecomment-664092447
Thanks @irairdon work for this, can you check and fix @merlimat comments? thanks again
----------------------------------------------------------------
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
[GitHub] [pulsar] wolfstudy commented on pull request #7572: [CPP] Fix segment crashes that caused by race condition of timer in cpp client
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on pull request #7572:
URL: https://github.com/apache/pulsar/pull/7572#issuecomment-664794135
----------------------------------------------------------------
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
[GitHub] [pulsar] jiazhai commented on pull request #7572: [CPP] Fix segment crashes that caused by race condition of timer in cpp client
Posted by GitBox <gi...@apache.org>.
jiazhai commented on pull request #7572:
URL: https://github.com/apache/pulsar/pull/7572#issuecomment-662381226
@merlimat @irairdon @klwilson227 @sijie Would you please help review the changes?
The former change would still have a chance to access null ptr, add the lock for `timer` to make the access safer.
----------------------------------------------------------------
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
[GitHub] [pulsar] jiazhai commented on pull request #7572: [CPP] Fix segment crashes that caused by race condition of timer in cpp client
Posted by GitBox <gi...@apache.org>.
jiazhai commented on pull request #7572:
URL: https://github.com/apache/pulsar/pull/7572#issuecomment-662833516
/pulsarbot run-failure-checks
----------------------------------------------------------------
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
[GitHub] [pulsar] jiazhai commented on pull request #7572: Kevin Wilson changes to fix segment crashes in pulsar
Posted by GitBox <gi...@apache.org>.
jiazhai commented on pull request #7572:
URL: https://github.com/apache/pulsar/pull/7572#issuecomment-660486944
Thanks @irairdon for this great fix. Seems your code is outdated with the latest master code, and caused some unnecessary changes in file `pulsar-client-cpp/lib/ClientConnection.cc`. Would you please help rebase your branch with the latest master branch？
----------------------------------------------------------------
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
[GitHub] [pulsar] wolfstudy merged pull request #7572: [CPP] Fix segment crashes that caused by race condition of timer in cpp client
Posted by GitBox <gi...@apache.org>.
wolfstudy merged pull request #7572:
URL: https://github.com/apache/pulsar/pull/7572
----------------------------------------------------------------
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
[GitHub] [pulsar] jiazhai commented on pull request #7572: [CPP] Fix segment crashes that caused by race condition of timer in cpp client
Posted by GitBox <gi...@apache.org>.
jiazhai commented on pull request #7572:
URL: https://github.com/apache/pulsar/pull/7572#issuecomment-662441662
/pulsarbot run-failure-checks
----------------------------------------------------------------
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
[GitHub] [pulsar] jiazhai commented on pull request #7572: [CPP] Fix segment crashes that caused by race condition of timer in cpp client
Posted by GitBox <gi...@apache.org>.
jiazhai commented on pull request #7572:
URL: https://github.com/apache/pulsar/pull/7572#issuecomment-662911300
/pulsarbot run-failure-checks
----------------------------------------------------------------
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
[GitHub] [pulsar] merlimat commented on a change in pull request #7572: Kevin Wilson changes to fix segment crashes in pulsar
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #7572:
URL: https://github.com/apache/pulsar/pull/7572#discussion_r456227158
##########
File path: pulsar-client-cpp/lib/ClientConnection.cc
##########
@@ -289,13 +286,14 @@ void ClientConnection::startConsumerStatsTimer(std::vector<uint64_t> consumerSta
consumerStatsRequests.push_back(it->first);
}
- DeadlineTimerPtr timer = consumerStatsRequestTimer_;
- if (timer) {
- timer->expires_from_now(operationsTimeout_);
- timer->async_wait(std::bind(&ClientConnection::handleConsumerStatsTimeout, shared_from_this(),
- std::placeholders::_1, consumerStatsRequests));
+ // If the close operation has reset the consumerStatsRequestTimer_ then the use_count will be zero
+ // Check if we have a timer still before we set the request timer to pop again.
+ if (consumerStatsRequestTimer_.use_count() > 0) {
Review comment:
```suggestion
if (consumerStatsRequestTimer_) {
```
##########
File path: pulsar-client-cpp/lib/ClientConnection.cc
##########
@@ -1344,8 +1324,12 @@ void ClientConnection::handleKeepAliveTimeout() {
havePendingPingRequest_ = true;
sendCommand(Commands::newPing());
- keepAliveTimer_->expires_from_now(boost::posix_time::seconds(KeepAliveIntervalInSeconds));
- keepAliveTimer_->async_wait(std::bind(&ClientConnection::handleKeepAliveTimeout, shared_from_this()));
+ // If the close operation has already called the keepAliveTimer_.reset() then the use_count will be zero And we do not attempt
+ // to dereference the pointer.
+ if (keepAliveTimer_.use_count() > 0) {
Review comment:
```suggestion
if (keepAliveTimer_) {
```
----------------------------------------------------------------
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