MB-21540: ep-engine: Remove abort()

Final set of changes to remove abort() from ep_engine production code;
replacing with exceptions where a dynamic error is still required.

All non-test ep-engine code is now abort-free :)

Change-Id: I9702b6bbaf28267b696498067318e78af0988002
Reviewed-on: http://review.couchbase.org/69376
Reviewed-by: Daniel Owen <owend@couchbase.com>
Reviewed-by: Jim Walker <jim@couchbase.com>
Tested-by: buildbot <build@couchbase.com>

diff --git a/src/common.h b/src/common.h
index 7ab46b7..0e213b6 100644 (file)
--- a/src/common.h
+++ b/src/common.h
@@ -27,7 +27,6 @@
 #include <math.h>
 #include <memcached/engine.h>
 #include <platform/platform.h>
-#include <cJSON.h>
 
 #include <stdint.h>
 #include <stdlib.h>
@@ -270,14 +269,4 @@ bool sorted(ForwardIterator first, ForwardIterator last, Compare compare) {
     return is_sorted;
 }
 
-inline const std::string getJSONObjString(const cJSON *i) {
-    if (i == NULL) {
-        return "";
-    }
-    if (i->type != cJSON_String) {
-        abort();
-    }
-    return i->valuestring;
-}
-
 #endif  // SRC_COMMON_H_

diff --git a/src/configuration.cc b/src/configuration.cc
index 44baa5b..6b31be2 100644 (file)
--- a/src/configuration.cc
+++ b/src/configuration.cc
@@ -399,8 +399,10 @@ bool Configuration::parseConfiguration(const char *str,
                 case DT_FLOAT:
                     setParameter(items[ii].key, *items[ii].value.dt_float);
                     break;
-                default:
-                    abort();
+                case DT_CONFIGFILE:
+                    throw std::logic_error("Configuration::parseConfiguration: "
+                            "Unexpected DT_CONFIGFILE element after parse_config");
+                    break;
                 }
             }
 

diff --git a/src/couch-kvstore/couch-kvstore.cc b/src/couch-kvstore/couch-kvstore.cc
index 6fcbc86..c8b5551 100644 (file)
--- a/src/couch-kvstore/couch-kvstore.cc
+++ b/src/couch-kvstore/couch-kvstore.cc
@@ -1104,9 +1104,10 @@ ScanContext* CouchKVStore::initScanContext(std::shared_ptr<Callback<GetValue> >
     DbInfo info;
     errorCode = couchstore_db_info(db, &info);
     if (errorCode != COUCHSTORE_SUCCESS) {
-        LOG(EXTENSION_LOG_WARNING, "Failed to read DB info for backfill");
         closeDatabaseHandle(db);
-        abort();
+        throw std::runtime_error("Failed to read DB info for backfill. vb:" +
+                                 std::to_string(vbid) + " rev:" +
+                                 std::to_string(rev));
     }
 
     uint64_t count = 0;

diff --git a/src/ep.cc b/src/ep.cc
index 08b1a54..844f678 100644 (file)
--- a/src/ep.cc
+++ b/src/ep.cc
@@ -757,12 +757,15 @@ ENGINE_ERROR_CODE EventuallyPersistentStore::addTempItemForBgFetch(
     switch(rv) {
         case ADD_NOMEM:
             return ENGINE_ENOMEM;
+
         case ADD_EXISTS:
         case ADD_UNDEL:
         case ADD_SUCCESS:
         case ADD_TMP_AND_BG_FETCH:
             // Since the hashtable bucket is locked, we shouldn't get here
-            abort();
+            throw std::logic_error("EventuallyPersistentStore::addTempItemForBgFetch: "
+                    "Invalid result from addTempItem: " + std::to_string(rv));
+
         case ADD_BG_FETCH:
             lock.unlock();
             bgFetch(key, vb->getId(), cookie, metadataOnly);
@@ -1105,8 +1108,9 @@ ENGINE_ERROR_CODE EventuallyPersistentStore::addTAPBackfillItem(
         ret = ENGINE_NOT_MY_VBUCKET;
         break;
     case NEED_BG_FETCH:
-        // SET on a non-active vbucket should not require a bg_metadata_fetch.
-        abort();
+        throw std::logic_error("EventuallyPersistentStore::addTAPBackfillItem: "
+                "SET on a non-active vbucket should not require a "
+                "bg_metadata_fetch.");
     }
 
     return ret;
@@ -2332,7 +2336,10 @@ EventuallyPersistentStore::statsVKey(const std::string &key,
             case ADD_SUCCESS:
             case ADD_TMP_AND_BG_FETCH:
                 // Since the hashtable bucket is locked, we shouldn't get here
-                abort();
+                throw std::logic_error("EventuallyPersistentStore::statsVKey: "
+                        "Invalid result from unlocked_addTempItem (" +
+                        std::to_string(rv) + ")");
+
             case ADD_BG_FETCH:
                 {
                     ++bgFetchQueue;
@@ -2729,7 +2736,8 @@ ENGINE_ERROR_CODE EventuallyPersistentStore::deleteItem(const std::string &key,
     case NEED_BG_FETCH:
         // We already figured out if a bg fetch is requred for a full-evicted
         // item above.
-        abort();
+        throw std::logic_error("EventuallyPersistentStore::deleteItem: "
+                "Unexpected NEEDS_BG_FETCH from unlocked_softDelete");
     }
     return ret;
 }

diff --git a/src/ep_engine.cc b/src/ep_engine.cc
index 358f3b4..f33f513 100644 (file)
--- a/src/ep_engine.cc
+++ b/src/ep_engine.cc
@@ -2503,11 +2503,12 @@ inline uint16_t EventuallyPersistentEngine::doWalkTapQueue(const void *cookie,
             *es = &connection->opaqueCommandCode;
             *nes = sizeof(connection->opaqueCommandCode);
             break;
+
         default:
-            LOG(EXTENSION_LOG_WARNING,
-                "%s Unknown VBucketEvent message type %d\n",
-                connection->logHeader(), ev.event);
-            abort();
+            throw std::logic_error("EventuallyPersistentEngine::doWalkTapQueue:"
+                    " Unknown VBucketEvent message type:" +
+                    std::to_string(ev.event) + " for connection:" +
+                    connection->logHeader());
         }
         return ev.event;
     }

diff --git a/src/flusher.cc b/src/flusher.cc
index cf2d1e4..252179c 100644 (file)
--- a/src/flusher.cc
+++ b/src/flusher.cc
@@ -82,8 +82,9 @@ static bool validTransition(enum flusher_state from,
     case stopped:
         return false;
     }
-    // THis should be impossible (unless someone added new states)
-    abort();
+    // This should be impossible (unless someone added new states)
+    throw std::logic_error("flusher::validTransition: called with invalid "
+                           "from:" + std::to_string(from));
 }
 
 const char * Flusher::stateName(enum flusher_state st) const {

diff --git a/src/forest-kvstore/forest-kvstore.cc b/src/forest-kvstore/forest-kvstore.cc
index 8ad847a..9eeb81b 100644 (file)
--- a/src/forest-kvstore/forest-kvstore.cc
+++ b/src/forest-kvstore/forest-kvstore.cc
@@ -86,9 +86,9 @@ ForestKVStore::ForestKVStore(KVStoreConfig &config) :
         char *ptr = NULL;
         uint64_t revNum = strtoull(revNumStr.c_str(), &ptr, 10);
         if (revNum == 0) {
-            LOG(EXTENSION_LOG_WARNING,
-                "Invalid revision number obtained for database file");
-            abort();
+            throw std::runtime_error("ForestKVStore: Invalid revision (" +
+                                     std::to_string(revNum) +
+                                     ") obtained for database file");
         }
 
         if (revNum > dbFileRevNum) {
@@ -111,10 +111,9 @@ ForestKVStore::ForestKVStore(KVStoreConfig &config) :
 
     status = fdb_open(&dbFileHandle, dbFile.str().c_str(), &fileConfig);
     if (status != FDB_RESULT_SUCCESS) {
-        LOG(EXTENSION_LOG_WARNING,
-            "Opening the database file instance failed with error: %s\n",
-            fdb_error_msg(status));
-        abort();
+        throw std::runtime_error(std::string("ForestKVStore: Opening the "
+                "database file instance failed with error:") +
+                fdb_error_msg(status));
     }
 
     fdb_kvs_handle *kvsHandle = NULL;
@@ -130,10 +129,9 @@ ForestKVStore::ForestKVStore(KVStoreConfig &config) :
     status = fdb_kvs_open_default(dbFileHandle, &vbStateHandle, &kvsConfig);
 
     if (status != FDB_RESULT_SUCCESS) {
-        LOG(EXTENSION_LOG_WARNING,
-            "Opening the vbucket state KV store instance failed "
-            "with error: %s\n", fdb_error_msg(status));
-        abort();
+        throw std::runtime_error(std::string("ForestKVStore: Opening the "
+                "vbucket state KV store instance failed with error:") +
+                fdb_error_msg(status));
     }
 
     cachedVBStates.reserve(maxVbuckets);

diff --git a/src/kvstore.h b/src/kvstore.h
index a85842d..364aa11 100644 (file)
--- a/src/kvstore.h
+++ b/src/kvstore.h
@@ -20,6 +20,7 @@
 
 #include "config.h"
 
+#include <cJSON.h>
 #include <cstring>
 #include <map>
 #include <list>
@@ -677,6 +678,17 @@ protected:
     void *dbHandle;
 };
 
+inline const std::string getJSONObjString(const cJSON *i) {
+    if (i == NULL) {
+        return "";
+    }
+    if (i->type != cJSON_String) {
+        throw std::invalid_argument("getJSONObjString: type of object (" +
+                                    std::to_string(i->type) +
+                                    ") is not cJSON_String");
+    }
+    return i->valuestring;
+}
 
 
 #endif  // SRC_KVSTORE_H_

diff --git a/src/mutation_log.cc b/src/mutation_log.cc
index efe16f6..e86dd69 100644 (file)
--- a/src/mutation_log.cc
+++ b/src/mutation_log.cc
@@ -148,7 +148,8 @@ int64_t getFileSize(file_handle_t fd) {
     if (GetFileSizeEx(fd, &li)) {
         return li.QuadPart;
     }
-    abort();
+    throw std::system_error(GetLastError(), std::system_category(),
+                            "getFileSize: failed");
 }
 
 #else
@@ -919,7 +920,8 @@ bool MutationLogHarvester::load() {
             // nothing in particular
             break;
         default:
-            abort();
+            throw std::logic_error("MutationLogHarvester::load: Invalid log "
+                    "entry type:" + std::to_string(le->type()));
         }
     }
     return clean;

diff --git a/src/warmup.cc b/src/warmup.cc
index 058babb..706f9c7 100644 (file)
--- a/src/warmup.cc
+++ b/src/warmup.cc
@@ -239,7 +239,9 @@ void LoadStorageKVPairCallback::callback(GetValue &val) {
                 }
             }
 
-            switch (vb->ht.insert(*i, policy, shouldEject(), val.isPartial())) {
+            const auto res = vb->ht.insert(*i, policy, shouldEject(),
+                                           val.isPartial());
+            switch (res) {
             case NOMEM:
                 if (retry == 2) {
                     if (hasPurged) {
@@ -269,7 +271,9 @@ void LoadStorageKVPairCallback::callback(GetValue &val) {
                 succeeded = true;
                 break;
             default:
-                abort();
+                throw std::logic_error("LoadStorageKVPairCallback::callback: "
+                        "Unexpected result from HashTable::insert: " +
+                        std::to_string(res));
             }
         } while (!succeeded && retry-- > 0);
 
@@ -909,8 +913,7 @@ void Warmup::done()
 }
 
 void Warmup::step() {
-    try {
-        switch (state.getState()) {
+    switch (state.getState()) {
         case WarmupState::Initialize:
             scheduleInitialize();
             break;
@@ -939,15 +942,8 @@ void Warmup::step() {
             scheduleCompletion();
             break;
         default:
-            LOG(EXTENSION_LOG_WARNING,
-                "Internal error.. Illegal warmup state %d", state.getState());
-            abort();
-        }
-    } catch(std::runtime_error &e) {
-        std::stringstream ss;
-        ss << "Exception in warmup loop: " << e.what() << std::endl;
-        LOG(EXTENSION_LOG_WARNING, "%s", ss.str().c_str());
-        abort();
+            throw std::logic_error("Warmup::step: illegal warmup state:" +
+                                   std::to_string(state.getState()));
     }
 }