Bug fix/applicationserver stop (#9414)

arangod/Agency/AgencyComm.cpp

@@ -1346,7 +1346,7 @@ AgencyCommResult AgencyComm::sendWithFailover(arangodb::rest::RequestType method
 
   auto waitSomeTime = [&waitInterval, &result]() -> bool {
     // Returning true means timeout because of shutdown:
-    if (!application_features::ApplicationServer::isRetryOK()) {
+    if (application_features::ApplicationServer::isStopping()) {
       LOG_TOPIC("53e58", INFO, Logger::AGENCYCOMM)
           << "Unsuccessful AgencyComm: Timeout because of shutdown "
           << "errorCode: " << result.errorCode()
@@ -1366,7 +1366,7 @@ AgencyCommResult AgencyComm::sendWithFailover(arangodb::rest::RequestType method
     }
 
     // Check again for shutdown, since some time has passed:
-    if (!application_features::ApplicationServer::isRetryOK()) {
+    if (application_features::ApplicationServer::isStopping()) {
       LOG_TOPIC("afe45", INFO, Logger::AGENCYCOMM)
           << "Unsuccessful AgencyComm: Timeout because of shutdown "
           << "errorCode: " << result.errorCode()
@@ -1411,30 +1411,12 @@ AgencyCommResult AgencyComm::sendWithFailover(arangodb::rest::RequestType method
 
     // Some reporting:
     if (tries > 20) {
-      auto serverState = application_features::ApplicationServer::server->state();
+      std::string serverState = application_features::ApplicationServer::server->stringifyState();
-      std::string serverStateStr;
-      switch (serverState) {
-        case arangodb::application_features::ServerState::UNINITIALIZED:
-        case arangodb::application_features::ServerState::IN_COLLECT_OPTIONS:
-        case arangodb::application_features::ServerState::IN_VALIDATE_OPTIONS:
-        case arangodb::application_features::ServerState::IN_PREPARE:
-        case arangodb::application_features::ServerState::IN_START:
-          serverStateStr = "in startup";
-          break;
-        case arangodb::application_features::ServerState::IN_WAIT:
-          serverStateStr = "running";
-          break;
-        case arangodb::application_features::ServerState::IN_STOP:
-        case arangodb::application_features::ServerState::IN_UNPREPARE:
-        case arangodb::application_features::ServerState::STOPPED:
-        case arangodb::application_features::ServerState::ABORT:
-          serverStateStr = "in shutdown";
-      }
       LOG_TOPIC("2f181", INFO, Logger::AGENCYCOMM)
           << "Flaky agency communication to " << endpoint
           << ". Unsuccessful consecutive tries: " << tries << " (" << elapsed
           << "s). Network checks advised."
-          << " Server " << serverStateStr << ".";
+          << " Server " << serverState << ".";
     }
 
     if (1 < tries) {

arangod/Cluster/ActionDescription.h

@@ -56,7 +56,7 @@ enum ActionState {
  * @brief Action description for mainenance actions
  *
  * This structure holds once initialized constant parameters of a maintenance
- * action. Members are declared const, thus thread safety guards are ommited.
+ * action. Members are declared const, thus thread safety guards are omitted.
  */
 struct ActionDescription {
  public:

arangod/Cluster/AgencyCallback.cpp

@@ -130,7 +130,7 @@ bool AgencyCallback::executeByCallbackOrTimeout(double maxTimeout) {
   // One needs to acquire the mutex of the condition variable
   // before entering this function!
   if (!_cv.wait(static_cast<uint64_t>(maxTimeout * 1000000.0)) &&
-      application_features::ApplicationServer::isRetryOK()) {
+      !application_features::ApplicationServer::isStopping()) {
     LOG_TOPIC("1514e", DEBUG, Logger::CLUSTER)
         << "Waiting done and nothing happended. Refetching to be sure";
     // mop: watches have not triggered during our sleep...recheck to be sure

arangod/Cluster/ClusterComm.cpp

@@ -661,7 +661,7 @@ std::unique_ptr<ClusterCommResult> ClusterComm::syncRequest(
   communicator()->addRequest(std::move(newRequest));
 
   while (!sharedData->wasSignaled
-         && application_features::ApplicationServer::isRetryOK()) {
+         && !application_features::ApplicationServer::isStopping()) {
     sharedData->cv.wait(100000);
   } // while
 

arangod/Cluster/ClusterInfo.cpp

@@ -1479,7 +1479,7 @@ Result ClusterInfo::createDatabaseCoordinator(  // create database
 
       agencyCallback->executeByCallbackOrTimeout(getReloadServerListTimeout() / interval);
 
-      if (!application_features::ApplicationServer::isRetryOK()) {
+      if (application_features::ApplicationServer::isStopping()) {
         return Result(TRI_ERROR_CLUSTER_TIMEOUT);
       }
     }
@@ -1583,7 +1583,7 @@ Result ClusterInfo::dropDatabaseCoordinator(  // drop database
 
       agencyCallback->executeByCallbackOrTimeout(interval);
 
-      if (!application_features::ApplicationServer::isRetryOK()) {
+      if (application_features::ApplicationServer::isStopping()) {
         return Result(TRI_ERROR_CLUSTER_TIMEOUT);
       }
     }
@@ -2084,10 +2084,10 @@ Result ClusterInfo::createCollectionsCoordinator(std::string const& databaseName
       }
     }
 
-  } while (application_features::ApplicationServer::isRetryOK());
+  } while (!application_features::ApplicationServer::isStopping());
   // If we get here we are not allowed to retry.
   // The loop above does not contain a break
-  TRI_ASSERT(!application_features::ApplicationServer::isRetryOK());
+  TRI_ASSERT(application_features::ApplicationServer::isStopping());
   for (auto const& info : infos) {
     events::CreateCollection(databaseName, info.name, TRI_ERROR_SHUTTING_DOWN);
   }
@@ -2265,7 +2265,7 @@ Result ClusterInfo::dropCollectionCoordinator(  // drop collection
 
       agencyCallback->executeByCallbackOrTimeout(interval);
 
-      if (!application_features::ApplicationServer::isRetryOK()) {
+      if (application_features::ApplicationServer::isStopping()) {
         events::DropCollection(dbName, collectionID, TRI_ERROR_CLUSTER_TIMEOUT);
         return Result(TRI_ERROR_CLUSTER_TIMEOUT);
       }
@@ -3269,7 +3269,7 @@ Result ClusterInfo::dropIndexCoordinator(  // drop index
 
       agencyCallback->executeByCallbackOrTimeout(interval);
 
-      if (!application_features::ApplicationServer::isRetryOK()) {
+      if (application_features::ApplicationServer::isStopping()) {
         return Result(TRI_ERROR_CLUSTER_TIMEOUT);
       }
     }

arangod/Cluster/FollowerInfo.cpp

@@ -183,11 +183,11 @@ Result FollowerInfo::add(ServerID const& sid) {
     }
     std::this_thread::sleep_for(std::chrono::microseconds(500000));
   } while (TRI_microtime() < startTime + 3600 &&
-           application_features::ApplicationServer::isRetryOK());
+           !application_features::ApplicationServer::isStopping());
   // This is important, give it 1h if needed. We really do not want to get
   // into the position to not accept a shard getting-in-sync just because
   // we cannot talk to the agency temporarily.
-  int errorCode = (application_features::ApplicationServer::isRetryOK()) ? TRI_ERROR_CLUSTER_AGENCY_COMMUNICATION_FAILED : TRI_ERROR_SHUTTING_DOWN;
+  int errorCode = (application_features::ApplicationServer::isStopping()) ? TRI_ERROR_SHUTTING_DOWN : TRI_ERROR_CLUSTER_AGENCY_COMMUNICATION_FAILED;
   std::string errorMessage = "unable to add follower in agency, timeout in agency CAS operation for key " + path + ": " + TRI_errno_string(errorCode);
   LOG_TOPIC("6295b", ERR, Logger::CLUSTER) << errorMessage;
 
@@ -325,7 +325,8 @@ Result FollowerInfo::remove(ServerID const& sid) {
     }
     std::this_thread::sleep_for(std::chrono::microseconds(500000));
   } while (TRI_microtime() < startTime + 7200 &&
-           application_features::ApplicationServer::isRetryOK());
+           !application_features::ApplicationServer::isStopping());
+  
   // This is important, give it 2h if needed. We really do not want to get
   // into the position to fail to drop a follower, just because we cannot
   // talk to the agency temporarily. The worst would be to drop the follower
@@ -337,7 +338,7 @@ Result FollowerInfo::remove(ServerID const& sid) {
   // rollback:
   _followers = _oldFollowers;
   
-  int errorCode = (application_features::ApplicationServer::isRetryOK()) ? TRI_ERROR_CLUSTER_AGENCY_COMMUNICATION_FAILED : TRI_ERROR_SHUTTING_DOWN;
+  int errorCode = (application_features::ApplicationServer::isStopping()) ? TRI_ERROR_SHUTTING_DOWN : TRI_ERROR_CLUSTER_AGENCY_COMMUNICATION_FAILED;
   std::string errorMessage = "unable to remove follower from agency, timeout in agency CAS operation for key " + path + ": " + TRI_errno_string(errorCode);
   LOG_TOPIC("a0dcc", ERR, Logger::CLUSTER) << errorMessage;
 

lib/ApplicationFeatures/ApplicationFeature.cpp

@@ -34,7 +34,7 @@ namespace application_features {
 ApplicationFeature::ApplicationFeature(ApplicationServer& server, std::string const& name)
     : _server(server),
       _name(name),
-      _state(ApplicationServer::FeatureState::UNINITIALIZED),
+      _state(State::UNINITIALIZED),
       _enabled(true),
       _optional(false),
       _requiresElevatedPrivileges(false),

lib/ApplicationFeatures/ApplicationFeature.h

@@ -44,6 +44,16 @@ class ApplicationFeature {
   ApplicationFeature(ApplicationServer& server, std::string const& name);
 
   virtual ~ApplicationFeature();
+  
+  enum class State {
+    UNINITIALIZED,
+    INITIALIZED,
+    VALIDATED,
+    PREPARED,
+    STARTED,
+    STOPPED,
+    UNPREPARED
+  };
 
   // return the feature's name
   std::string const& name() const { return _name; }
@@ -51,7 +61,7 @@ class ApplicationFeature {
   bool isOptional() const { return _optional; }
   bool isRequired() const { return !_optional; }
 
-  ApplicationServer::FeatureState state() const { return _state; }
+  State state() const { return _state; }
 
   // whether or not the feature is enabled
   bool isEnabled() const { return _enabled; }
@@ -177,7 +187,7 @@ class ApplicationFeature {
  private:
   // set a feature's state. this method should be called by the
   // application server only
-  void state(ApplicationServer::FeatureState state) { _state = state; }
+  void state(State state) { _state = state; }
 
   // determine all direct and indirect ancestors of a feature
   void determineAncestors();
@@ -205,7 +215,7 @@ class ApplicationFeature {
   std::unordered_set<std::string> _onlyEnabledWith;
 
   // state of feature
-  ApplicationServer::FeatureState _state;
+  State _state;
 
   // whether or not the feature is enabled
   bool _enabled;

lib/ApplicationFeatures/ApplicationServer.cpp

@@ -26,6 +26,7 @@
 #include "ApplicationFeatures/PrivilegeFeature.h"
 #include "Basics/ConditionLocker.h"
 #include "Basics/Result.h"
+#include "Basics/ScopeGuard.h"
 #include "Basics/StringUtils.h"
 #include "Basics/process-utils.h"
 #include "Logger/Logger.h"
@@ -49,9 +50,8 @@ ApplicationServer* ApplicationServer::server = nullptr;
 
 ApplicationServer::ApplicationServer(std::shared_ptr<ProgramOptions> options,
                                      const char* binaryPath)
-    : _state(ServerState::UNINITIALIZED),
+    : _state(State::UNINITIALIZED),
       _options(options),
-      _stopping(false),
       _binaryPath(binaryPath) {
   // register callback function for failures
   fail = failCallback;
@@ -78,6 +78,35 @@ ApplicationServer::~ApplicationServer() {
   ApplicationServer::server = nullptr;
 }
 
+bool ApplicationServer::isPrepared() {
+  if (server == nullptr) {
+    return false;
+  }
+
+  auto tmp = server->state();
+  return tmp == State::IN_START || 
+         tmp == State::IN_WAIT ||
+         tmp == State::IN_SHUTDOWN ||
+         tmp == State::IN_STOP;
+}
+
+bool ApplicationServer::isStopping() {
+  if (server == nullptr) {
+    return false;
+  }
+
+  auto tmp = server->state();
+  return isStoppingState(tmp);
+}
+
+bool ApplicationServer::isStoppingState(State state) {
+  return state == State::IN_SHUTDOWN ||
+         state == State::IN_STOP ||
+         state == State::IN_UNPREPARE ||
+         state == State::STOPPED ||
+         state == State::ABORTED; 
+}
+
 void ApplicationServer::throwFeatureNotFoundException(std::string const& name) {
   THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_INTERNAL,
                                  "unknown feature '" + name + "'");
@@ -126,7 +155,7 @@ void ApplicationServer::disableFeatures(std::vector<std::string> const& names, b
 // will take ownership of the feature object and destroy it in its
 // destructor
 void ApplicationServer::addFeature(ApplicationFeature* feature) {
-  TRI_ASSERT(feature->state() == FeatureState::UNINITIALIZED);
+  TRI_ASSERT(feature->state() == ApplicationFeature::State::UNINITIALIZED);
   _features.emplace(feature->name(), feature);
 }
 
@@ -173,8 +202,8 @@ void ApplicationServer::run(int argc, char* argv[]) {
 
   // collect options from all features
   // in this phase, all features are order-independent
-  _state.store(ServerState::IN_COLLECT_OPTIONS, std::memory_order_relaxed);
+  _state.store(State::IN_COLLECT_OPTIONS, std::memory_order_relaxed);
-  reportServerProgress(ServerState::IN_COLLECT_OPTIONS);
+  reportServerProgress(State::IN_COLLECT_OPTIONS);
   collectOptions();
 
   // setup dependency, but ignore any failure for now
@@ -193,8 +222,8 @@ void ApplicationServer::run(int argc, char* argv[]) {
   _options->seal();
 
   // validate options of all features
-  _state.store(ServerState::IN_VALIDATE_OPTIONS, std::memory_order_relaxed);
+  _state.store(State::IN_VALIDATE_OPTIONS, std::memory_order_relaxed);
-  reportServerProgress(ServerState::IN_VALIDATE_OPTIONS);
+  reportServerProgress(State::IN_VALIDATE_OPTIONS);
   validateOptions();
 
   // setup and validate all feature dependencies
@@ -212,8 +241,8 @@ void ApplicationServer::run(int argc, char* argv[]) {
   // furthermore, they must not write any files under elevated privileges
   // if they want other features to access them, or if they want to access
   // these files with dropped privileges
-  _state.store(ServerState::IN_PREPARE, std::memory_order_relaxed);
+  _state.store(State::IN_PREPARE, std::memory_order_relaxed);
-  reportServerProgress(ServerState::IN_PREPARE);
+  reportServerProgress(State::IN_PREPARE);
   prepare();
 
   // turn off all features that depend on other features that have been
@@ -225,62 +254,79 @@ void ApplicationServer::run(int argc, char* argv[]) {
   dropPrivilegesPermanently();
 
   // start features. now features are allowed to start threads, write files etc.
-  _state.store(ServerState::IN_START, std::memory_order_relaxed);
+  _state.store(State::IN_START, std::memory_order_relaxed);
-  reportServerProgress(ServerState::IN_START);
+  reportServerProgress(State::IN_START);
   start();
 
   // wait until we get signaled the shutdown request
-  _state.store(ServerState::IN_WAIT, std::memory_order_relaxed);
+  _state.store(State::IN_WAIT, std::memory_order_relaxed);
-  reportServerProgress(ServerState::IN_WAIT);
+  reportServerProgress(State::IN_WAIT);
   wait();
 
+  // beginShutdown is called asynchronously ----------
+
   // stop all features
-  _state.store(ServerState::IN_STOP, std::memory_order_relaxed);
+  _state.store(State::IN_STOP, std::memory_order_relaxed);
-  reportServerProgress(ServerState::IN_STOP);
+  reportServerProgress(State::IN_STOP);
   stop();
 
   // unprepare all features
-  _state.store(ServerState::IN_UNPREPARE, std::memory_order_relaxed);
+  _state.store(State::IN_UNPREPARE, std::memory_order_relaxed);
-  reportServerProgress(ServerState::IN_UNPREPARE);
+  reportServerProgress(State::IN_UNPREPARE);
   unprepare();
 
   // stopped
-  _state.store(ServerState::STOPPED, std::memory_order_relaxed);
+  _state.store(State::STOPPED, std::memory_order_relaxed);
-  reportServerProgress(ServerState::STOPPED);
+  reportServerProgress(State::STOPPED);
 }
 
 // signal the server to shut down
 void ApplicationServer::beginShutdown() {
+  // fetch the old state, check if somebody already called shutdown, and only
+  // proceed if not.
+  State old = State::UNINITIALIZED;
+  do {
+    old = state();
+    if (isStoppingState(old)) {
+      // beginShutdown already called, nothing to do now
+      return;
+    }
+    // try to enter the new state, but make sure nobody changed it in between
+  } while (!_state.compare_exchange_weak(old, State::IN_SHUTDOWN, std::memory_order_relaxed));
+  
   LOG_TOPIC("c7911", TRACE, Logger::STARTUP) << "ApplicationServer::beginShutdown";
 
-  bool old = _stopping.exchange(true);
+  // make sure that we advance the state when we get out of here
+  auto waitAborter = scopeGuard([this]() { 
+    CONDITION_LOCKER(guard, _shutdownCondition);
+
+    _abortWaiting = true;
+    guard.signal();
+  });
+  
+  // now we can execute the actual shutdown sequence
 
   // fowards the begin shutdown signal to all features
-  if (!old) {
+  for (auto it = _orderedFeatures.rbegin(); it != _orderedFeatures.rend(); ++it) {
-    for (auto it = _orderedFeatures.rbegin(); it != _orderedFeatures.rend(); ++it) {
+    if ((*it)->isEnabled()) {
-      if ((*it)->isEnabled()) {
+      LOG_TOPIC("e181f", TRACE, Logger::STARTUP) << (*it)->name() << "::beginShutdown";
-        LOG_TOPIC("e181f", TRACE, Logger::STARTUP) << (*it)->name() << "::beginShutdown";
+      try {
-        try {
+        (*it)->beginShutdown();
-          (*it)->beginShutdown();
+      } catch (std::exception const& ex) {
-        } catch (std::exception const& ex) {
+        LOG_TOPIC("b2cf4", ERR, Logger::STARTUP)
-          LOG_TOPIC("b2cf4", ERR, Logger::STARTUP)
+            << "caught exception during beginShutdown of feature '"
-              << "caught exception during beginShutdown of feature '"
+            << (*it)->name() << "': " << ex.what();
-              << (*it)->name() << "': " << ex.what();
+      } catch (...) {
-        } catch (...) {
+        LOG_TOPIC("3f708", ERR, Logger::STARTUP)
-          LOG_TOPIC("3f708", ERR, Logger::STARTUP)
+            << "caught unknown exception during beginShutdown of feature '"
-              << "caught unknown exception during beginShutdown of feature '"
+            << (*it)->name() << "'";
-              << (*it)->name() << "'";
-        }
       }
     }
   }
-
-  CONDITION_LOCKER(guard, _shutdownCondition);
-  guard.signal();
 }
 
 void ApplicationServer::shutdownFatalError() {
-  reportServerProgress(ServerState::ABORT);
+  reportServerProgress(State::ABORTED);
 }
 
 // return VPack options, with optional filters applied to filter
@@ -384,7 +430,7 @@ void ApplicationServer::validateOptions() {
     if (feature->isEnabled()) {
       LOG_TOPIC("fa73c", TRACE, Logger::STARTUP) << feature->name() << "::validateOptions";
       feature->validateOptions(_options);
-      feature->state(FeatureState::VALIDATED);
+      feature->state(ApplicationFeature::State::VALIDATED);
       reportFeatureProgress(_state.load(std::memory_order_relaxed), feature->name());
     }
   }
@@ -487,7 +533,7 @@ void ApplicationServer::setupDependencies(bool failOnMissing) {
   for (auto it = features.begin(); it != features.end(); /* no hoisting */) {
     if ((*it)->isEnabled()) {
       // keep feature
-      (*it)->state(FeatureState::INITIALIZED);
+      (*it)->state(ApplicationFeature::State::INITIALIZED);
       ++it;
     } else {
       // remove feature
@@ -566,7 +612,7 @@ void ApplicationServer::prepare() {
       try {
         LOG_TOPIC("d4e57", TRACE, Logger::STARTUP) << feature->name() << "::prepare";
         feature->prepare();
-        feature->state(FeatureState::PREPARED);
+        feature->state(ApplicationFeature::State::PREPARED);
       } catch (std::exception const& ex) {
         LOG_TOPIC("37921", ERR, Logger::STARTUP)
             << "caught exception during prepare of feature '" << feature->name()
@@ -606,7 +652,7 @@ void ApplicationServer::start() {
 
     try {
       feature->start();
-      feature->state(FeatureState::STARTED);
+      feature->state(ApplicationFeature::State::STARTED);
       reportFeatureProgress(_state.load(std::memory_order_relaxed), feature->name());
     } catch (basics::Exception const& ex) {
       res.reset(
@@ -643,13 +689,13 @@ void ApplicationServer::start() {
         if (!feature->isEnabled()) {
           continue;
         }
-        if (feature->state() == FeatureState::STARTED) {
+        if (feature->state() == ApplicationFeature::State::STARTED) {
           LOG_TOPIC("e5cfd", TRACE, Logger::STARTUP)
               << "forcefully stopping feature '" << feature->name() << "'";
           try {
             feature->beginShutdown();
             feature->stop();
-            feature->state(FeatureState::STOPPED);
+            feature->state(ApplicationFeature::State::STOPPED);
           } catch (...) {
             // ignore errors on shutdown
             LOG_TOPIC("13223", TRACE, Logger::STARTUP)
@@ -661,12 +707,12 @@ void ApplicationServer::start() {
       // try to unprepare all feature that we just started
       for (auto it = _orderedFeatures.rbegin(); it != _orderedFeatures.rend(); ++it) {
         auto feature = *it;
-        if (feature->state() == FeatureState::STOPPED) {
+        if (feature->state() == ApplicationFeature::State::STOPPED) {
           LOG_TOPIC("6ba4f", TRACE, Logger::STARTUP)
               << "forcefully unpreparing feature '" << feature->name() << "'";
           try {
             feature->unprepare();
-            feature->state(FeatureState::UNPREPARED);
+            feature->state(ApplicationFeature::State::UNPREPARED);
           } catch (...) {
             // ignore errors on shutdown
             LOG_TOPIC("7d68f", TRACE, Logger::STARTUP)
@@ -707,7 +753,7 @@ void ApplicationServer::stop() {
           << "caught unknown exception during stop of feature '"
           << feature->name() << "'";
     }
-    feature->state(FeatureState::STOPPED);
+    feature->state(ApplicationFeature::State::STOPPED);
     reportFeatureProgress(_state.load(std::memory_order_relaxed), feature->name());
   }
 }
@@ -733,7 +779,7 @@ void ApplicationServer::unprepare() {
           << "caught unknown exception during unprepare of feature '"
           << feature->name() << "'";
     }
-    feature->state(FeatureState::UNPREPARED);
+    feature->state(ApplicationFeature::State::UNPREPARED);
     reportFeatureProgress(_state.load(std::memory_order_relaxed), feature->name());
   }
 }
@@ -741,9 +787,18 @@ void ApplicationServer::unprepare() {
 void ApplicationServer::wait() {
   LOG_TOPIC("f86df", TRACE, Logger::STARTUP) << "ApplicationServer::wait";
 
-  while (!_stopping) {
+  // wait here until beginShutdown has been called and finished
+  while (true) {
+    // wait until somebody calls beginShutdown and it finishes
     CONDITION_LOCKER(guard, _shutdownCondition);
-    guard.wait(100000);
+    
+    if (_abortWaiting) {
+      // yippieh!
+      break;
+    }
+
+    using namespace std::chrono_literals;
+    guard.wait(100ms);
   }
 }
 
@@ -789,14 +844,44 @@ void ApplicationServer::dropPrivilegesPermanently() {
   _privilegesDropped = true;
 }
 
-void ApplicationServer::reportServerProgress(ServerState state) {
+void ApplicationServer::reportServerProgress(State state) {
   for (auto reporter : _progressReports) {
     reporter._state(state);
   }
 }
 
-void ApplicationServer::reportFeatureProgress(ServerState state, std::string const& name) {
+void ApplicationServer::reportFeatureProgress(State state, std::string const& name) {
   for (auto reporter : _progressReports) {
     reporter._feature(state, name);
   }
 }
+
+char const* ApplicationServer::stringifyState() const { 
+  switch (_state.load()) {
+    case State::UNINITIALIZED: 
+      return "uninitialized";
+    case State::IN_COLLECT_OPTIONS:
+      return "in collect options";
+    case State::IN_VALIDATE_OPTIONS:
+      return "in validate options";
+    case State::IN_PREPARE:
+      return "in prepare";
+    case State::IN_START:
+      return "in start";
+    case State::IN_WAIT:
+      return "in wait";
+    case State::IN_SHUTDOWN:
+      return "in beginShutdown";
+    case State::IN_STOP:
+      return "in stop";
+    case State::IN_UNPREPARE:
+      return "in unprepare";
+    case State::STOPPED:
+      return "in stopped";
+    case State::ABORTED:
+      return "in aborted";
+  }
+  // we should never get here
+  TRI_ASSERT(false);
+  return "unknown";
+}

lib/ApplicationFeatures/ApplicationServer.h

@@ -39,26 +39,6 @@ class ProgramOptions;
 namespace application_features {
 class ApplicationFeature;
 
-// handled i.e. in WindowsServiceFeature.cpp
-enum class ServerState {
-  UNINITIALIZED,
-  IN_COLLECT_OPTIONS,
-  IN_VALIDATE_OPTIONS,
-  IN_PREPARE,
-  IN_START,
-  IN_WAIT,
-  IN_STOP,
-  IN_UNPREPARE,
-  STOPPED,
-  ABORT
-};
-
-class ProgressHandler {
- public:
-  std::function<void(ServerState)> _state;
-  std::function<void(ServerState, std::string const& featureName)> _feature;
-};
-
 // the following phases exists:
 //
 // `collectOptions`
@@ -119,39 +99,38 @@ class ApplicationServer {
   ApplicationServer& operator=(ApplicationServer const&) = delete;
 
  public:
-  enum class FeatureState {
+  // handled i.e. in WindowsServiceFeature.cpp
+  enum class State : int {
     UNINITIALIZED,
-    INITIALIZED,
+    IN_COLLECT_OPTIONS,
-    VALIDATED,
+    IN_VALIDATE_OPTIONS,
-    PREPARED,
+    IN_PREPARE,
-    STARTED,
+    IN_START,
+    IN_WAIT,
+    IN_SHUTDOWN,
+    IN_STOP,
+    IN_UNPREPARE,
     STOPPED,
-    UNPREPARED
+    ABORTED
   };
+  
+  class ProgressHandler {
+   public:
+    std::function<void(State)> _state;
+    std::function<void(State, std::string const& featureName)> _feature;
+   };
 
   static ApplicationServer* server;
 
-  static bool isStopping() {
+  
-    return server != nullptr && server->_stopping.load();
+  /// @brief whether or not the server has made it as least as far as the IN_START state
-  }
+  static bool isPrepared();
+  
+  /// @brief whether or not the server has made it as least as far as the IN_SHUTDOWN state
+  static bool isStopping();
 
-  // Today this static function is a duplicate of isStopping().  The
+  /// @brief whether or not state is the shutting down state or further (i.e. stopped, aborted etc.)
-  //  function name 'isStopping()' is defined in other classes and
+  static bool isStoppingState(State state);
-  //  can cause scope confusion.  It also causes confusion as to when
-  //  the application versus an individual feature or thread has begun
-  //  stopping.  This function is intended to be used within communication
-  //  retry loops where infinite retries have previously blocked clean
-  //  "stopping".
-  static bool isRetryOK() { return !isStopping(); }
-
-  static bool isPrepared() {
-    if (server != nullptr) {
-      ServerState tmp = server->_state.load(std::memory_order_relaxed);
-      return tmp == ServerState::IN_START || tmp == ServerState::IN_WAIT ||
-             tmp == ServerState::IN_STOP;
-    }
-    return false;
-  }
 
   // returns the feature with the given name if known
   // throws otherwise
@@ -192,6 +171,9 @@ class ApplicationServer {
   std::string helpSection() const { return _helpSection; }
   bool helpShown() const { return !_helpSection.empty(); }
 
+  /// @brief stringify the internal state
+  char const* stringifyState() const;
+
   // adds a feature to the application server. the application server
   // will take ownership of the feature object and destroy it in its
   // destructor
@@ -237,7 +219,7 @@ class ApplicationServer {
   std::shared_ptr<options::ProgramOptions> options() const { return _options; }
 
   // return the server state
-  ServerState state() const { return _state; }
+  State state() const { return _state; }
 
   void addReporter(ProgressHandler reporter) {
     _progressReports.emplace_back(reporter);
@@ -257,7 +239,7 @@ class ApplicationServer {
     return lookupFeature<T>(T::name());
   }
 
-  char const* getBinaryPath() { return _binaryPath; }
+  char const* getBinaryPath() const { return _binaryPath; }
 
   void registerStartupCallback(std::function<void()> const& callback) {
     _startupCallbacks.emplace_back(callback);
@@ -323,12 +305,12 @@ class ApplicationServer {
   void dropPrivilegesTemporarily();
   void dropPrivilegesPermanently();
 
-  void reportServerProgress(ServerState);
+  void reportServerProgress(State);
-  void reportFeatureProgress(ServerState, std::string const&);
+  void reportFeatureProgress(State, std::string const&);
 
  private:
   // the current state
-  std::atomic<ServerState> _state;
+  std::atomic<State> _state;
 
   // the shared program options
   std::shared_ptr<options::ProgramOptions> _options;
@@ -339,11 +321,12 @@ class ApplicationServer {
   // features order for prepare/start
   std::vector<ApplicationFeature*> _orderedFeatures;
 
-  // will be signalled when the application server is asked to shut down
+  // will be signaled when the application server is asked to shut down
   basics::ConditionVariable _shutdownCondition;
 
-  // stop flag. this is being changed by calling beginShutdown
+  /// @brief the condition variable protects access to this flag
-  std::atomic<bool> _stopping;
+  /// the flag is set to true when beginShutdown finishes
+  bool _abortWaiting = false;
 
   // whether or not privileges have been dropped permanently
   bool _privilegesDropped = false;

lib/ApplicationFeatures/CommunicationPhase.h

@@ -36,16 +36,17 @@ class CommunicationFeaturePhase : public ApplicationFeaturePhase {
    */
   bool getCommAllowed() {
     switch (state()) {
-      case ApplicationServer::FeatureState::UNINITIALIZED:
+      case ApplicationFeature::State::UNINITIALIZED:
-      case ApplicationServer::FeatureState::INITIALIZED:
+      case ApplicationFeature::State::INITIALIZED:
-      case ApplicationServer::FeatureState::VALIDATED:
+      case ApplicationFeature::State::VALIDATED:
-      case ApplicationServer::FeatureState::PREPARED:
+      case ApplicationFeature::State::PREPARED:
-      case ApplicationServer::FeatureState::STARTED:
+      case ApplicationFeature::State::STARTED:
-      case ApplicationServer::FeatureState::STOPPED:
+      case ApplicationFeature::State::STOPPED:
         return true;
-      case ApplicationServer::FeatureState::UNPREPARED:
+      case ApplicationFeature::State::UNPREPARED:
-        return false;
+        break; 
     }
+
     return false;
   }
 };

lib/ApplicationFeatures/WindowsServiceFeature.cpp

@@ -382,7 +382,7 @@ void SetServiceStatus(DWORD dwCurrentState, DWORD dwWin32ExitCode,
 }
 
 //////////////////////////////////////////////////////////////////////////////
-/// @brief wrap ArangoDB server so we can properly emmit a status once we're
+/// @brief wrap ArangoDB server so we can properly emit a status once we're
 ///        really up and running.
 //////////////////////////////////////////////////////////////////////////////
 void WindowsServiceFeature::startupProgress() {
@@ -390,47 +390,54 @@ void WindowsServiceFeature::startupProgress() {
 }
 
 //////////////////////////////////////////////////////////////////////////////
-/// @brief wrap ArangoDB server so we can properly emmit a status once we're
+/// @brief wrap ArangoDB server so we can properly emit a status once we're
 ///        really up and running.
 //////////////////////////////////////////////////////////////////////////////
 void WindowsServiceFeature::startupFinished() {
-  // startup finished - signalize we're running.
+  // startup finished - signal that we're running.
   SetServiceStatus(SERVICE_RUNNING, NO_ERROR, 0, 0, 0);
 }
 
 //////////////////////////////////////////////////////////////////////////////
-/// @brief wrap ArangoDB server so we can properly emmit a status on shutdown
+/// @brief wrap ArangoDB server so we can properly emit a status on shutdown
 ///        starting
 //////////////////////////////////////////////////////////////////////////////
-void WindowsServiceFeature::shutDownBegins() {
+void WindowsServiceFeature::shutdownBegins() {
-  // startup finished - signalize we're running.
+  auto shutdownNoted = _shutdownNoted.exchange(true);
+
+  if (shutdownNoted) {
+    // we were already called before. don't note the shutdown twice
+    return;
+  }
+
+  // startup finished - signal that we are shutting down
   SetServiceStatus(SERVICE_STOP_PENDING, NO_ERROR, 0, 0, 0);
 }
 
 //////////////////////////////////////////////////////////////////////////////
-/// @brief wrap ArangoDB server so we can properly emmit a status on shutdown
+/// @brief wrap ArangoDB server so we can properly emit a status on shutdown
 ///        starting
 //////////////////////////////////////////////////////////////////////////////
-void WindowsServiceFeature::shutDownComplete() {
+void WindowsServiceFeature::shutdownComplete() {
-  // startup finished - signalize we're running.
+  // startup finished - signal that we have shut down
   SetServiceStatus(SERVICE_STOPPED, NO_ERROR, 0, 0, 0);
 }
 
 //////////////////////////////////////////////////////////////////////////////
-/// @brief wrap ArangoDB server so we can properly emmit a status on shutdown
+/// @brief wrap ArangoDB server so we can properly emit a status on shutdown
-///        starting
+///        failed
 //////////////////////////////////////////////////////////////////////////////
-void WindowsServiceFeature::shutDownFailure() {
+void WindowsServiceFeature::shutdownFailure() {
-  // startup finished - signalize we're running.
+  // startup finished - signal that shutdown has failed
   SetServiceStatus(SERVICE_STOP, ERROR_SERVICE_SPECIFIC_ERROR, 0, 0, 1);
 }
 
 //////////////////////////////////////////////////////////////////////////////
-/// @brief wrap ArangoDB server so we can properly emmit a status on shutdown
+/// @brief wrap ArangoDB server so we can properly emit a status on shutdown
 ///        starting
 //////////////////////////////////////////////////////////////////////////////
 void WindowsServiceFeature::abortFailure(uint16_t exitCode) {
-  // startup finished - signalize we're running.
+  // startup finished - signal that the service has been aborted
   SetServiceStatus(SERVICE_STOP, ERROR_SERVICE_SPECIFIC_ERROR, 0, 0, exitCode);
 }
 
@@ -471,8 +478,10 @@ void WINAPI ServiceCtrl(DWORD dwCtrlCode) {
 }
 
 WindowsServiceFeature::WindowsServiceFeature(application_features::ApplicationServer& server)
-    : ApplicationFeature(server, "WindowsService"), _server(&server) {
+    : ApplicationFeature(server, "WindowsService"), 
-  _progress = 2;
+      _server(&server), 
+      _progress(2), 
+      _shutdownNoted(false) {
   setOptional(true);
   requiresElevatedPrivileges(true);
   startsAfter("GreetingsPhase");
@@ -502,7 +511,7 @@ void WindowsServiceFeature::collectOptions(std::shared_ptr<ProgramOptions> optio
                                                   arangodb::options::Flags::Command));
 
   options->addOption("--uninstall-service",
-                     "used to UNregister a service with windows",
+                     "used to unregister a service with windows",
                      new BooleanParameter(&_unInstallService),
                      arangodb::options::makeFlags(arangodb::options::Flags::Hidden,
                                                   arangodb::options::Flags::Command));
@@ -523,7 +532,7 @@ void WindowsServiceFeature::collectOptions(std::shared_ptr<ProgramOptions> optio
 
   options->addOption(
       "--servicectl-start-wait",
-      "command an already registered service to start and wait till its up",
+      "command an already registered service to start and wait till it's up",
       new BooleanParameter(&_startWaitService),
       arangodb::options::makeFlags(arangodb::options::Flags::Hidden,
                                    arangodb::options::Flags::Command));
@@ -536,7 +545,7 @@ void WindowsServiceFeature::collectOptions(std::shared_ptr<ProgramOptions> optio
 
   options->addOption(
       "--servicectl-stop-wait",
-      "command an already registered service to stop and wait till its gone",
+      "command an already registered service to stop and wait till it's gone",
       new BooleanParameter(&_stopWaitService),
       arangodb::options::makeFlags(arangodb::options::Flags::Hidden,
                                    arangodb::options::Flags::Command));
@@ -563,35 +572,36 @@ void WindowsServiceFeature::validateOptions(std::shared_ptr<ProgramOptions> opti
   } else if (_startAsService) {
     TRI_SetWindowsServiceAbortFunction(abortService);
 
-    ProgressHandler reporter{[this](ServerState state) {
+    ApplicationServer::ProgressHandler reporter{[this](ApplicationServer::State state) {
                                switch (state) {
-                                 case ServerState::IN_WAIT:
+                                 case ApplicationServer::State::IN_WAIT:
                                    this->startupFinished();
                                    break;
-                                 case ServerState::IN_STOP:
+                                 case ApplicationServer::State::IN_SHUTDOWN:
-                                   this->shutDownBegins();
+                                 case ApplicationServer::State::IN_STOP:
+                                   this->shutdownBegins();
                                    break;
-                                 case ServerState::IN_COLLECT_OPTIONS:
+                                 case ApplicationServer::State::IN_COLLECT_OPTIONS:
-                                 case ServerState::IN_VALIDATE_OPTIONS:
+                                 case ApplicationServer::State::IN_VALIDATE_OPTIONS:
-                                 case ServerState::IN_PREPARE:
+                                 case ApplicationServer::State::IN_PREPARE:
-                                 case ServerState::IN_START:
+                                 case ApplicationServer::State::IN_START:
                                    this->startupProgress();
                                    break;
-                                 case ServerState::ABORT:
+                                 case ApplicationServer::State::ABORTED:
-                                   this->shutDownFailure();
+                                   this->shutdownFailure();
                                    break;
-                                 case ServerState::UNINITIALIZED:
+                                 case ApplicationServer::State::UNINITIALIZED:
-                                 case ServerState::STOPPED:
+                                 case ApplicationServer::State::STOPPED:
                                    break;
                                }
                              },
-                             [this](application_features::ServerState state,
+                             [this](application_features::ApplicationServer::State state,
                                     std::string const& name) {
                                switch (state) {
-                                 case ServerState::IN_COLLECT_OPTIONS:
+                                 case ApplicationServer::State::IN_COLLECT_OPTIONS:
-                                 case ServerState::IN_VALIDATE_OPTIONS:
+                                 case ApplicationServer::State::IN_VALIDATE_OPTIONS:
-                                 case ServerState::IN_PREPARE:
+                                 case ApplicationServer::State::IN_PREPARE:
-                                 case ServerState::IN_START:
+                                 case ApplicationServer::State::IN_START:
                                    this->startupProgress();
                                    break;
                                  default:

lib/ApplicationFeatures/WindowsServiceFeature.h

@@ -24,6 +24,9 @@
 #define ARANGODB_APPLICATION_FEATURES_WINDOWS_SERVICE_FEATURE_H 1
 
 #include "ApplicationFeatures/ApplicationFeature.h"
+#include "ApplicationFeatures/ApplicationServer.h"
+
+#include <atomic>
 
 extern SERVICE_STATUS_HANDLE ServiceStatus;
 
@@ -49,9 +52,9 @@ class WindowsServiceFeature final : public application_features::ApplicationFeat
 
   void startupFinished();
 
-  void shutDownBegins();
+  void shutdownBegins();
-  void shutDownComplete();
+  void shutdownComplete();
-  void shutDownFailure();
+  void shutdownFailure();
   void abortFailure(uint16_t exitCode);
   static void abortService(uint16_t exitCode);
 
@@ -69,8 +72,11 @@ class WindowsServiceFeature final : public application_features::ApplicationFeat
 
  private:
   uint16_t _progress;
+
+  /// @brief flag that tells us whether we have been informed about the shutdown before
+  std::atomic<bool> _shutdownNoted;
 };
 
 }  // namespace arangodb
 
-#endif
+#endif

tests/Maintenance/MaintenanceFeatureMock.h

@@ -46,7 +46,7 @@ typedef std::vector<Expected> ExpectedVec_t;
 //
 // TestProgressHandler lets us know once ApplicationServer is ready
 //
-class TestProgressHandler : public arangodb::application_features::ProgressHandler {
+class TestProgressHandler : public arangodb::application_features::ApplicationServer::ProgressHandler {
 public:
   TestProgressHandler() {
     _serverReady=false;
@@ -59,15 +59,15 @@ public:
   }
 
 
-  void StateChange(arangodb::application_features::ServerState newState) {
+  void StateChange(arangodb::application_features::ApplicationServer::State newState) {
-    if (arangodb::application_features::ServerState::IN_WAIT == newState) {
+    if (arangodb::application_features::ApplicationServer::State::IN_WAIT == newState) {
       CONDITION_LOCKER(clock, _serverReadyCond);
       _serverReady = true;
       _serverReadyCond.broadcast();
     }
   }
 
-  void FeatureChange(arangodb::application_features::ServerState newState, std::string const &) {
+  void FeatureChange(arangodb::application_features::ApplicationServer::State newState, std::string const &) {
   }
 
   arangodb::basics::ConditionVariable _serverReadyCond;