Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(meta): fix null pointer while clearing environment variables after table was dropped #2181

Merged
merged 7 commits into from
Jan 16, 2025
Merged

fix(meta): fix null pointer while clearing environment variables after table was dropped #2181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
67c1b76
fix(meta): fix null pointer while clearing environment variables afte…
empiredan Jan 14, 2025
28e04ab
format
empiredan Jan 14, 2025
a288e73
add tests and fix IWYU
empiredan Jan 14, 2025
7d27795
refactor and add comments
empiredan Jan 14, 2025
3c44253
fix IWYU
empiredan Jan 15, 2025
706c9c8
fix clang-tidy
empiredan Jan 15, 2025
c26dab2
refactor
empiredan Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
174 changes: 121 additions & 53 deletions src/meta/server_state.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

// IWYU pragma: no_include <boost/detail/basic_pointerbuf.hpp>
#include <boost/algorithm/string/join.hpp>
#include <boost/algorithm/string/predicate.hpp>
#include <boost/lexical_cast.hpp>
// IWYU pragma: no_include <ext/alloc_traits.h>
#include <fmt/core.h>
Expand All @@ -40,6 +41,7 @@
#include <string>
#include <string_view>
#include <thread>
#include <type_traits>
#include <unordered_map>

#include "common/duplication_common.h"
Expand Down Expand Up @@ -3266,15 +3268,15 @@ void server_state::del_app_envs(const app_env_rpc &env_rpc)

void server_state::clear_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.clear_prefix) {
env_rpc.response().err = ERR_INVALID_PARAMETERS;
LOG_WARNING("clear app envs failed with invalid request");
return;
}

const std::string &prefix = request.clear_prefix;
const std::string &app_name = request.app_name;
const auto &prefix = request.clear_prefix;
const auto &app_name = request.app_name;
LOG_INFO("clear app envs for app({}) from remote({}): prefix = {}",
app_name,
env_rpc.remote_address(),
Expand All @@ -3283,79 +3285,145 @@ void server_state::clear_app_envs(const app_env_rpc &env_rpc)
app_info ainfo;
std::string app_path;
{
FAIL_POINT_INJECT_NOT_RETURN_F(
"clear_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("clear 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("clear 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;
} else {
ainfo = *(reinterpret_cast<app_info *>(app.get()));
app_path = get_app_path(*app);
}

if (app->status == app_status::AS_DROPPING) {
LOG_WARNING("clear 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;
}

ainfo = *app;
app_path = get_app_path(*app);
}

if (ainfo.envs.empty()) {
LOG_INFO("no key need to delete");
env_rpc.response().hint_message = "no key need to delete";
LOG_INFO("no key needs to be deleted for app({})", app_name);
env_rpc.response().err = ERR_OK;
env_rpc.response().hint_message = "no key needs to be deleted";
return;
}

std::set<std::string> erase_keys;
std::ostringstream oss;
oss << "deleted keys:";
std::set<std::string> deleted_keys;
std::string deleted_keys_info("deleted keys:");

if (prefix.empty()) {
// ignore prefix
for (auto &kv : ainfo.envs) {
oss << std::endl << " " << kv.first;
// Empty prefix means deleting all environments.
for (const auto &[key, _] : ainfo.envs) {
fmt::format_to(std::back_inserter(deleted_keys_info), "\n {}", key);
}
ainfo.envs.clear();
} else {
// acquire key
for (const auto &pair : ainfo.envs) {
const std::string &key = pair.first;
// normal : key = prefix.xxx
if (key.size() > prefix.size() + 1) {
if (key.substr(0, prefix.size()) == prefix && key.at(prefix.size()) == '.') {
erase_keys.emplace(key);
}
// The full prefix is the prefix plus the separator dot(.).
const size_t full_prefix_len = prefix.size() + sizeof('.');
for (const auto &[key, _] : ainfo.envs) {
// The key is not the target if it is shorter than, or just has the same length
// as the full prefix.
if (key.size() <= full_prefix_len) {
continue;
}

// The key is not the target if the prefix is not matched.
if (!boost::algorithm::starts_with(key, prefix)) {
continue;
}

// The key is not the target if the separator is not dot(.).
if (key[prefix.size()] != '.') {
continue;
}

deleted_keys.emplace(key);
}
// erase
for (const auto &key : erase_keys) {
oss << std::endl << " " << key;

for (const auto &key : deleted_keys) {
fmt::format_to(std::back_inserter(deleted_keys_info), "\n {}", key);
ainfo.envs.erase(key);
}
}

if (!prefix.empty() && erase_keys.empty()) {
// no need update app_info
LOG_INFO("no key need to delete");
env_rpc.response().hint_message = "no key need to delete";
return;
} else {
env_rpc.response().hint_message = oss.str();
if (deleted_keys.empty()) {
LOG_INFO("no key needs to be deleted for app({})", app_name);
env_rpc.response().err = ERR_OK;
env_rpc.response().hint_message = "no key needs to be deleted";
return;
}
}

do_update_app_info(
app_path, ainfo, [this, app_name, prefix, erase_keys, env_rpc](error_code ec) {
CHECK_EQ_MSG(ec, ERR_OK, "update app info to remote storage failed");
env_rpc.response().hint_message = std::move(deleted_keys_info);

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, ',', '=');
if (prefix.empty()) {
app->envs.clear();
} else {
for (const auto &key : erase_keys) {
app->envs.erase(key);
}
do_update_app_info(app_path, ainfo, [this, app_name, deleted_keys, env_rpc](error_code ec) {
CHECK_EQ_MSG(ec, ERR_OK, "update app({}) info to remote storage failed", app_name);

zauto_write_lock l(_lock);

FAIL_POINT_INJECT_NOT_RETURN_F("clear_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("clear 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;
}

env_rpc.response().err = ERR_OK;

const auto &old_envs = dsn::utils::kv_map_to_string(app->envs, ',', '=');

if (deleted_keys.empty()) {
// `deleted_keys` would be empty only when `prefix` is empty. Therefore, empty
// `deleted_keys` means deleting all environments.
app->envs.clear();
} else {
for (const auto &key : deleted_keys) {
app->envs.erase(key);
}
std::string new_envs = dsn::utils::kv_map_to_string(app->envs, ',', '=');
LOG_INFO("app envs changed: old_envs = {}, new_envs = {}", old_envs, new_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);
});
}

namespace {
Expand Down
Loading
Loading