From fef664ff4c35956d6dc465c3bbe74fe193a36fbd Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Fri, 13 Dec 2024 18:16:29 +0800 Subject: [PATCH] fix(meta): fix null pointer while deleting environment variables after table was dropped --- src/meta/server_state.cpp | 129 ++++++++++++++++++++++++++------------ 1 file changed, 88 insertions(+), 41 deletions(-) diff --git a/src/meta/server_state.cpp b/src/meta/server_state.cpp index 8e68113f72..59dbd56077 100644 --- a/src/meta/server_state.cpp +++ b/src/meta/server_state.cpp @@ -25,6 +25,7 @@ */ // IWYU pragma: no_include +#include #include // IWYU pragma: no_include #include @@ -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 &keys = request.keys; - const std::vector &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 = 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 &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 = 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.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 = 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); }); }