fix(meta): fix null pointer while deleting environment variables afte…

…r table was dropped

src/meta/server_state.cpp

@@ -25,6 +25,7 @@
  */
 
 // IWYU pragma: no_include <boost/detail/basic_pointerbuf.hpp>
+#include <boost/algorithm/string/join.hpp>
 #include <boost/lexical_cast.hpp>
 // IWYU pragma: no_include <ext/alloc_traits.h>
 #include <fmt/core.h>
@@ -2986,17 +2987,17 @@ void server_state::do_update_app_info(const std::string &app_path,
 
 void server_state::set_app_envs(const app_env_rpc &env_rpc)
 {
-    const configuration_update_app_env_request &request = env_rpc.request();
+    const auto &request = env_rpc.request();
     if (!request.__isset.keys || !request.__isset.values ||
-        request.keys.size() != request.values.size() || request.keys.size() <= 0) {
+        request.keys.size() != request.values.size() || request.keys.empty()) {
         env_rpc.response().err = ERR_INVALID_PARAMETERS;
         LOG_WARNING("set app envs failed with invalid request");
         return;
     }
 
-    const std::vector<std::string> &keys = request.keys;
-    const std::vector<std::string> &values = request.values;
-    const std::string &app_name = request.app_name;
+    const auto &keys = request.keys;
+    const auto &values = request.values;
+    const auto &app_name = request.app_name;
 
     std::ostringstream os;
     for (size_t i = 0; i < keys.size(); ++i) {
@@ -3078,7 +3079,7 @@ void server_state::set_app_envs(const app_env_rpc &env_rpc)
             }
         });
 
-        std::shared_ptr<app_state> app = get_app(app_name);
+        auto app = get_app(app_name);
 
         // The table might be removed just before the callback function is invoked, thus we must
         // check if this table still exists.
@@ -3109,70 +3110,116 @@ void server_state::set_app_envs(const app_env_rpc &env_rpc)
 
 void server_state::del_app_envs(const app_env_rpc &env_rpc)
 {
-    const configuration_update_app_env_request &request = env_rpc.request();
-    if (!request.__isset.keys || request.keys.size() <= 0) {
+    const auto &request = env_rpc.request();
+    if (!request.__isset.keys || request.keys.empty()) {
         env_rpc.response().err = ERR_INVALID_PARAMETERS;
         LOG_WARNING("del app envs failed with invalid request");
         return;
     }
-    const std::vector<std::string> &keys = request.keys;
-    const std::string &app_name = request.app_name;
 
-    std::ostringstream os;
-    for (int i = 0; i < keys.size(); i++) {
-        if (i != 0)
-            os << ",";
-        os << keys[i];
-    }
+    const auto &keys = request.keys;
+    const auto &app_name = request.app_name;
+
     LOG_INFO("del app envs for app({}) from remote({}): keys = {}",
              app_name,
              env_rpc.remote_address(),
-             os.str());
+             boost::join(keys, ","));
 
     app_info ainfo;
     std::string app_path;
     {
+        FAIL_POINT_INJECT_NOT_RETURN_F("del_app_envs_failed", [app_name, this](std::string_view s) {
+            zauto_write_lock l(_lock);
+
+            if (s == "not_found") {
+                CHECK_EQ(_exist_apps.erase(app_name), 1);
+                return;
+            }
+
+            if (s == "dropping") {
+                gutil::FindOrDie(_exist_apps, app_name)->status = app_status::AS_DROPPING;
+                return;
+            }
+        });
+
         zauto_read_lock l(_lock);
-        std::shared_ptr<app_state> app = get_app(app_name);
-        if (app == nullptr) {
-            LOG_WARNING("del app envs failed with invalid app_name({})", app_name);
-            env_rpc.response().err = ERR_INVALID_PARAMETERS;
-            env_rpc.response().hint_message = "invalid app name";
+
+        const auto &app = get_app(app_name);
+        if (!app) {
+            LOG_WARNING("del app envs failed since app_name({}) cannot be found", app_name);
+            env_rpc.response().err = ERR_APP_NOT_EXIST;
+            env_rpc.response().hint_message = "app cannot be found";
+            return;
+        }
+
+        if (app->status == app_status::AS_DROPPING) {
+            LOG_WARNING("del app envs failed since app(name={}, id={}) is being dropped",
+                        app_name,
+                        app->app_id);
+            env_rpc.response().err = ERR_BUSY_DROPPING;
+            env_rpc.response().hint_message = "app is being dropped";
             return;
-        } else {
-            ainfo = *(reinterpret_cast<app_info *>(app.get()));
-            app_path = get_app_path(*app);
         }
+
+            ainfo = *app;
+            app_path = get_app_path(*app);
     }
 
-    std::ostringstream oss;
-    oss << "deleted keys:";
-    int deleted = 0;
+    std::string deleted_keys_info("deleted keys:");
+    size_t deleted_count = 0;
     for (const auto &key : keys) {
-        if (ainfo.envs.erase(key) > 0) {
-            oss << std::endl << "    " << key;
-            deleted++;
+        if (ainfo.envs.erase(key) == 0) {
+            continue;
         }
+
+            deleted_keys_info += fmt::format("\n    {}", key);
+            ++deleted_count;
     }
 
-    if (deleted == 0) {
-        LOG_INFO("no key need to delete");
-        env_rpc.response().hint_message = "no key need to delete";
+    if (deleted_count == 0) {
+        std::string hint_message("no key need to delete");
+        LOG_INFO(hint_message);
+        env_rpc.response().hint_message = std::move(hint_message);
         return;
-    } else {
-        env_rpc.response().hint_message = oss.str();
-    }
+    } 
+
+        env_rpc.response().hint_message = std::move(deleted_keys_info);
 
     do_update_app_info(app_path, ainfo, [this, app_name, keys, env_rpc](error_code ec) {
-        CHECK_EQ_MSG(ec, ERR_OK, "update app info to remote storage failed");
+        CHECK_EQ_MSG(ec, ERR_OK, "update app({}) info to remote storage failed", app_name);
 
         zauto_write_lock l(_lock);
-        std::shared_ptr<app_state> app = get_app(app_name);
-        std::string old_envs = dsn::utils::kv_map_to_string(app->envs, ',', '=');
+
+        FAIL_POINT_INJECT_NOT_RETURN_F("del_app_envs_failed", [app_name, this](std::string_view s) {
+            if (s == "dropped_after") {
+                CHECK_EQ(_exist_apps.erase(app_name), 1);
+                return;
+            }
+        });
+
+        auto app = get_app(app_name);
+
+        // The table might be removed just before the callback function is invoked, thus we must
+        // check if this table still exists.
+        //
+        // TODO(wangdan): should make updates to remote storage sequential by supporting atomic
+        // set, otherwise update might be missing. For example, an update is setting the envs
+        // while another is dropping a table. The update setting the envs does not contain the
+        // dropped state. Once it is applied by remote storage after another update dropping
+        // the table, the state of the table would always be non-dropped on remote storage.
+        if (!app) {
+            LOG_ERROR("del app envs failed since app({}) has just been dropped", app_name);
+            env_rpc.response().err = ERR_APP_DROPPED;
+            env_rpc.response().hint_message = "app has just been dropped";
+            return;
+        }
+
+        const auto &old_envs = dsn::utils::kv_map_to_string(app->envs, ',', '=');
         for (const auto &key : keys) {
             app->envs.erase(key);
         }
-        std::string new_envs = dsn::utils::kv_map_to_string(app->envs, ',', '=');
+
+        const auto &new_envs = dsn::utils::kv_map_to_string(app->envs, ',', '=');
         LOG_INFO("app envs changed: old_envs = {}, new_envs = {}", old_envs, new_envs);
     });
 }