MB-21540: ep-engine: Remove abort() 76/69376/11
authorDave Rigby <daver@couchbase.com>
Tue, 1 Nov 2016 16:39:05 +0000 (16:39 +0000)
committerDave Rigby <daver@couchbase.com>
Fri, 18 Nov 2016 16:09:48 +0000 (16:09 +0000)
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>
src/common.h
src/configuration.cc
src/couch-kvstore/couch-kvstore.cc
src/ep.cc
src/ep_engine.cc
src/flusher.cc
src/forest-kvstore/forest-kvstore.cc
src/kvstore.h
src/mutation_log.cc
src/warmup.cc

index 7ab46b7..0e213b6 100644 (file)
@@ -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_
index 44baa5b..6b31be2 100644 (file)
@@ -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;
                 }
             }
 
index 6fcbc86..c8b5551 100644 (file)
@@ -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;
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;
 }
index 358f3b4..f33f513 100644 (file)
@@ -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;
     }
index cf2d1e4..252179c 100644 (file)
@@ -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 {
index 8ad847a..9eeb81b 100644 (file)
@@ -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);
index a85842d..364aa11 100644 (file)
@@ -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_
index efe16f6..e86dd69 100644 (file)
@@ -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;
index 058babb..706f9c7 100644 (file)
@@ -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()));
     }
 }