From f73b0b696133534995857392e364f21efaf589cf Mon Sep 17 00:00:00 2001 From: "A.J. Beamon" Date: Fri, 14 Jul 2017 16:28:04 -0700 Subject: [PATCH 1/2] fix: Move failureMonitorClient state to a reference counted object. This avoids a race condition in the fdbcli as its shutting down that can cause it to crash. --- fdbclient/FailureMonitorClient.actor.cpp | 39 ++++++++++++++---------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/fdbclient/FailureMonitorClient.actor.cpp b/fdbclient/FailureMonitorClient.actor.cpp index b263fb883a..46b0dd740f 100644 --- a/fdbclient/FailureMonitorClient.actor.cpp +++ b/fdbclient/FailureMonitorClient.actor.cpp @@ -22,12 +22,20 @@ #include "fdbrpc/FailureMonitor.h" #include "ClusterInterface.h" +struct FailureMonitorClientState : ReferenceCounted { + std::set knownAddrs; + double serverFailedTimeout; + + FailureMonitorClientState() { + serverFailedTimeout = CLIENT_KNOBS->FAILURE_TIMEOUT_DELAY; + } +}; + ACTOR Future failureMonitorClientLoop( SimpleFailureMonitor* monitor, ClusterInterface controller, - double* pServerFailedTimeout, - bool trackMyStatus, - std::set* knownAddrs) + Reference fmState, + bool trackMyStatus) { state Version version = 0; state Future request = Never(); @@ -37,7 +45,7 @@ ACTOR Future failureMonitorClientLoop( state double waitfor = 0; monitor->setStatus(controller.failureMonitoring.getEndpoint().address, FailureStatus(false)); - knownAddrs->insert( controller.failureMonitoring.getEndpoint().address ); + fmState->knownAddrs.insert( controller.failureMonitoring.getEndpoint().address ); //The cluster controller's address (controller.failureMonitoring.getEndpoint().address) is treated specially because we can declare that it is down independently //of the response from the cluster controller. It still needs to be in knownAddrs in case the cluster controller changes, so the next cluster controller resets its state @@ -51,14 +59,14 @@ ACTOR Future failureMonitorClientLoop( requestTimeout = Never(); if (reply.allOthersFailed) { // Reset all systems *not* mentioned in the reply to the default (failed) state - knownAddrs->erase( controller.failureMonitoring.getEndpoint().address ); + fmState->knownAddrs.erase( controller.failureMonitoring.getEndpoint().address ); std::set changedAddresses; for(int c=0; cknownAddrs) if (!changedAddresses.count( it )) monitor->setStatus( it, FailureStatus() ); - knownAddrs->clear(); + fmState->knownAddrs.clear(); } else { ASSERT( version != 0 ); } @@ -66,20 +74,20 @@ ACTOR Future failureMonitorClientLoop( if( monitor->getState( controller.failureMonitoring.getEndpoint() ).isFailed() ) TraceEvent("FailureMonitoringServerUp").detail("OldServer",controller.id()); monitor->setStatus( controller.failureMonitoring.getEndpoint().address, FailureStatus(false) ); - knownAddrs->insert( controller.failureMonitoring.getEndpoint().address ); + fmState->knownAddrs.insert( controller.failureMonitoring.getEndpoint().address ); //if (version != reply.failureInformationVersion) // printf("Client '%s': update from %lld to %lld (%d changes, aof=%d)\n", g_network->getLocalAddress().toString().c_str(), version, reply.failureInformationVersion, reply.changes.size(), reply.allOthersFailed); version = reply.failureInformationVersion; - *pServerFailedTimeout = reply.considerServerFailedTimeoutMS * .001; + fmState->serverFailedTimeout = reply.considerServerFailedTimeoutMS * .001; for(int c=0; cgetLocalAddress().toString().c_str(), reply.changes[c].address.toString().c_str(), reply.changes[c].status.failed ? "Failed" : "OK"); monitor->setStatus( reply.changes[c].address, reply.changes[c].status ); if (reply.changes[c].status != FailureStatus()) - knownAddrs->insert( reply.changes[c].address ); + fmState->knownAddrs.insert( reply.changes[c].address ); else - knownAddrs->erase( reply.changes[c].address ); + fmState->knownAddrs.erase( reply.changes[c].address ); ASSERT( reply.changes[c].address != controller.failureMonitoring.getEndpoint().address || !reply.changes[c].status.failed ); } before = now(); @@ -91,7 +99,7 @@ ACTOR Future failureMonitorClientLoop( requestTimeout = Never(); TraceEvent(SevWarn, "FailureMonitoringServerDown").detail("OldServerID",controller.id()); monitor->setStatus( controller.failureMonitoring.getEndpoint().address, FailureStatus(true) ); - knownAddrs->erase( controller.failureMonitoring.getEndpoint().address ); + fmState->knownAddrs.erase( controller.failureMonitoring.getEndpoint().address ); } when( Void _ = wait( nextRequest ) ) { g_network->setCurrentTask(TaskDefaultDelay); @@ -111,7 +119,7 @@ ACTOR Future failureMonitorClientLoop( req.senderStatus = FailureStatus(false); request = controller.failureMonitoring.getReply( req, TaskFailureMonitor ); if(!controller.failureMonitoring.getEndpoint().isLocal()) - requestTimeout = delay( *pServerFailedTimeout, TaskFailureMonitor ); + requestTimeout = delay( fmState->serverFailedTimeout, TaskFailureMonitor ); } } } @@ -125,11 +133,10 @@ ACTOR Future failureMonitorClientLoop( ACTOR Future failureMonitorClient( Reference>> ci, bool trackMyStatus ) { state SimpleFailureMonitor* monitor = static_cast( &IFailureMonitor::failureMonitor() ); - state std::set knownAddrs; - state double serverFailedTimeout = CLIENT_KNOBS->FAILURE_TIMEOUT_DELAY; + state Reference fmState = Reference(new FailureMonitorClientState()); loop { - state Future client = ci->get().present() ? failureMonitorClientLoop(monitor, ci->get().get(), &serverFailedTimeout, trackMyStatus, &knownAddrs) : Void(); + state Future client = ci->get().present() ? failureMonitorClientLoop(monitor, ci->get().get(), fmState, trackMyStatus) : Void(); Void _ = wait( ci->onChange() ); } } \ No newline at end of file From eee492a05bd897bc0b214ab35fcfffad58a3301f Mon Sep 17 00:00:00 2001 From: Alec Grieser Date: Fri, 14 Jul 2017 16:46:08 -0700 Subject: [PATCH 2/2] fix build issue from Notified.h not being shuffled in vcxproj files --- fdbclient/Notified.h | 4 ++-- fdbclient/fdbclient.vcxproj | 3 ++- flow/flow.vcxproj | 1 - flow/flow.vcxproj.filters | 3 +-- 4 files changed, 5 insertions(+), 6 deletions(-) diff --git a/fdbclient/Notified.h b/fdbclient/Notified.h index 2890b9cb41..fd32476a38 100644 --- a/fdbclient/Notified.h +++ b/fdbclient/Notified.h @@ -18,8 +18,8 @@ * limitations under the License. */ -#ifndef FLOW_NOTIFIED_H -#define FLOW_NOTIFIED_H +#ifndef FDBCLIENT_NOTIFIED_H +#define FDBCLIENT_NOTIFIED_H #pragma once #include "FDBTypes.h" diff --git a/fdbclient/fdbclient.vcxproj b/fdbclient/fdbclient.vcxproj index f4b0723505..01071cb2a6 100644 --- a/fdbclient/fdbclient.vcxproj +++ b/fdbclient/fdbclient.vcxproj @@ -57,6 +57,7 @@ + @@ -201,4 +202,4 @@ - \ No newline at end of file + diff --git a/flow/flow.vcxproj b/flow/flow.vcxproj index 63d63194ad..9581936338 100644 --- a/flow/flow.vcxproj +++ b/flow/flow.vcxproj @@ -71,7 +71,6 @@ - diff --git a/flow/flow.vcxproj.filters b/flow/flow.vcxproj.filters index 91f8139364..9b8b9fdd73 100644 --- a/flow/flow.vcxproj.filters +++ b/flow/flow.vcxproj.filters @@ -64,9 +64,8 @@ - - \ No newline at end of file +