From 08352a025c97d7177c50d7d6dad34f278ee14917 Mon Sep 17 00:00:00 2001 From: neverchanje Date: Sat, 15 Aug 2020 09:18:30 +0800 Subject: [PATCH 1/5] refactor(http): replace service/method with simple HTTP path --- include/dsn/http/http_server.h | 66 ++++++----------------------- src/http/http_call_registry.h | 67 ++++++++++++++++++++++++++++++ src/http/http_server.cpp | 44 +++++++++----------- src/http/root_http_service.h | 15 +++---- src/http/test/http_server_test.cpp | 26 ++++++------ src/meta/meta_http_service.cpp | 4 +- 6 files changed, 117 insertions(+), 105 deletions(-) create mode 100644 src/http/http_call_registry.h diff --git a/include/dsn/http/http_server.h b/include/dsn/http/http_server.h index 6e8e231772..a1ceaff204 100644 --- a/include/dsn/http/http_server.h +++ b/include/dsn/http/http_server.h @@ -21,8 +21,7 @@ struct http_request { static error_with parse(dsn::message_ex *m); - // http://ip:port// - std::pair service_method; + std::string path; // std::unordered_map query_args; blob body; @@ -51,48 +50,24 @@ struct http_response message_ptr to_message(message_ex *req) const; }; +typedef std::function http_callback; + +// Defines the structure of an HTTP call. +struct http_call +{ + std::string path; + std::string help; + http_callback callback; +}; + class http_service { public: - typedef std::function http_callback; - virtual ~http_service() = default; virtual std::string path() const = 0; - void register_handler(std::string path, http_callback cb, std::string help) - { - _cb_map.emplace(std::move(path), std::make_pair(std::move(cb), std::move(help))); - } - - void call(const http_request &req, http_response &resp) - { - auto it = _cb_map.find(req.service_method.second); - if (it != _cb_map.end()) { - it->second.first(req, resp); - } else { - resp.status_code = http_status_code::not_found; - resp.body = std::string("method not found for \"") + req.service_method.second + "\""; - } - } - - struct method_help_entry - { - std::string name; - std::string help; - }; - std::vector get_help() const - { - std::vector ret; - ret.reserve(_cb_map.size()); - for (const auto &method : _cb_map) { - ret.push_back({method.first, method.second.second}); - } - return ret; - } - -private: - std::map> _cb_map; + void register_handler(std::string path, http_callback cb, std::string help); }; class http_server : public serverlet @@ -106,23 +81,6 @@ class http_server : public serverlet void serve(message_ex *msg); - struct service_method_help_entry - { - std::string name; - std::string method; - std::string help; - }; - std::vector get_help() const - { - std::vector ret; - for (const auto &service : _service_map) { - for (const auto &method : service.second->get_help()) { - ret.push_back({service.first, method.name, method.help}); - } - } - return ret; - } - private: std::map> _service_map; }; diff --git a/src/http/http_call_registry.h b/src/http/http_call_registry.h new file mode 100644 index 0000000000..05b3588476 --- /dev/null +++ b/src/http/http_call_registry.h @@ -0,0 +1,67 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#pragma once + +#include +#include + +namespace dsn { + +// A singleton registry for all the HTTP calls +class http_call_registry : public utils::singleton +{ +public: + std::shared_ptr find(const std::string &path) const + { + std::lock_guard guard(_mu); + auto it = _call_map.find(path); + if (it == _call_map.end()) { + return nullptr; + } + return it->second; + } + + void remove(const std::string &path) + { + std::lock_guard guard(_mu); + _call_map.erase(path); + } + + void add(std::shared_ptr call) + { + std::lock_guard guard(_mu); + _call_map[call->path] = call; + } + + std::vector> list_all_calls() const + { + std::lock_guard guard(_mu); + + std::vector> ret; + for (const auto &kv : _call_map) { + ret.push_back(kv.second); + } + return ret; + } + +private: + mutable std::mutex _mu; + std::map> _call_map; +}; + +} // namespace dsn diff --git a/src/http/http_server.cpp b/src/http/http_server.cpp index ed8cf9027d..ed3831517c 100644 --- a/src/http/http_server.cpp +++ b/src/http/http_server.cpp @@ -12,6 +12,7 @@ #include "pprof_http_service.h" #include "perf_counter_http_service.h" #include "uri_decoder.h" +#include "http_call_registry.h" namespace dsn { @@ -34,6 +35,18 @@ namespace dsn { } } +void http_service::register_handler(std::string path, http_callback cb, std::string help) +{ + auto call = std::make_shared(); + call->path = this->path(); + if (!path.empty()) { + call->path += "/" + std::move(path); + } + call->callback = std::move(cb); + call->help = std::move(help); + http_call_registry::instance().add(std::move(call)); +} + http_server::http_server(bool start /*default=true*/) : serverlet("http_server") { if (!start) { @@ -45,7 +58,7 @@ http_server::http_server(bool start /*default=true*/) : serverlet(" tools::register_message_header_parser(NET_HDR_HTTP, {"GET ", "POST"}); // add builtin services - add_service(new root_http_service(this)); + add_service(new root_http_service()); #ifdef DSN_ENABLE_GPERF add_service(new pprof_http_service()); @@ -63,12 +76,12 @@ void http_server::serve(message_ex *msg) resp.body = fmt::format("failed to parse request: {}", res.get_error()); } else { const http_request &req = res.get_value(); - auto it = _service_map.find(req.service_method.first); - if (it != _service_map.end()) { - it->second->call(req, resp); + std::shared_ptr call = http_call_registry::instance().find(req.path); + if (call != nullptr) { + call->callback(req, resp); } else { resp.status_code = http_status_code::not_found; - resp.body = fmt::format("service not found for \"{}\"", req.service_method.first); + resp.body = fmt::format("service not found for \"{}\"", req.path); } } @@ -131,26 +144,7 @@ void http_server::add_service(http_service *service) if (!unresolved_path.empty() && *unresolved_path.crbegin() == '\0') { unresolved_path.pop_back(); } - std::vector args; - boost::split(args, unresolved_path, boost::is_any_of("/")); - std::vector real_args; - for (std::string &arg : args) { - if (!arg.empty()) { - real_args.emplace_back(std::move(arg)); - } - } - if (real_args.size() == 0) { - ret.service_method = {"", ""}; - } else if (real_args.size() == 1) { - ret.service_method = {std::move(real_args[0]), ""}; - } else { - std::string method = std::move(real_args[1]); - for (int i = 2; i < real_args.size(); i++) { - method += '/'; - method += real_args[i]; - } - ret.service_method = {std::move(real_args[0]), std::move(method)}; - } + ret.path = std::move(unresolved_path); // find if there are method args (://?=&=) if (!unresolved_query.empty()) { diff --git a/src/http/root_http_service.h b/src/http/root_http_service.h index c7895e0a74..73a2ea5044 100644 --- a/src/http/root_http_service.h +++ b/src/http/root_http_service.h @@ -6,12 +6,14 @@ #include #include +#include "http_call_registry.h" + namespace dsn { class root_http_service : public http_service { public: - explicit root_http_service(http_server *server) : _server(server) + explicit root_http_service() { // url: ip:port/ register_handler("", @@ -28,19 +30,14 @@ class root_http_service : public http_service { utils::table_printer tp; std::ostringstream oss; - auto help_entries = _server->get_help(); - for (const auto &ent : help_entries) { - tp.add_row_name_and_data(std::string("/") + ent.name + (ent.method.empty() ? "" : "/") + - ent.method, - ent.help); + auto calls = http_call_registry::instance().list_all_calls(); + for (const auto &call : calls) { + tp.add_row_name_and_data(std::string("/") + call->path, call->help); } tp.output(oss, utils::table_printer::output_format::kJsonCompact); resp.body = oss.str(); resp.status_code = http_status_code::ok; } - -private: - http_server *_server; }; } // namespace dsn diff --git a/src/http/test/http_server_test.cpp b/src/http/test/http_server_test.cpp index 1d56a338fa..1801e1c043 100644 --- a/src/http/test/http_server_test.cpp +++ b/src/http/test/http_server_test.cpp @@ -18,17 +18,17 @@ TEST(http_server, parse_url) std::string url; error_code err; - std::pair result; + std::string path; } tests[] = { - {"http://127.0.0.1:34601", ERR_OK, {"", ""}}, - {"http://127.0.0.1:34601/", ERR_OK, {"", ""}}, - {"http://127.0.0.1:34601///", ERR_OK, {"", ""}}, - {"http://127.0.0.1:34601/threads", ERR_OK, {"threads", ""}}, - {"http://127.0.0.1:34601/threads/", ERR_OK, {"threads", ""}}, - {"http://127.0.0.1:34601//pprof/heap/", ERR_OK, {"pprof", "heap"}}, - {"http://127.0.0.1:34601//pprof///heap", ERR_OK, {"pprof", "heap"}}, - {"http://127.0.0.1:34601/pprof/heap/arg/", ERR_OK, {"pprof", "heap/arg"}}, - {"http://127.0.0.1:34601/pprof///heap///arg/", ERR_OK, {"pprof", "heap/arg"}}, + {"http://127.0.0.1:34601", ERR_OK, ""}, + {"http://127.0.0.1:34601/", ERR_OK, "/"}, + {"http://127.0.0.1:34601///", ERR_OK, "///"}, + {"http://127.0.0.1:34601/threads", ERR_OK, "/threads"}, + {"http://127.0.0.1:34601/threads/?detail", ERR_OK, "/threads/"}, + {"http://127.0.0.1:34601//pprof/heap/", ERR_OK, "//pprof/heap/"}, + {"http://127.0.0.1:34601//pprof///heap?detailed=true", ERR_OK, "//pprof///heap"}, + {"http://127.0.0.1:34601/pprof/heap/arg/", ERR_OK, "/pprof/heap/arg/"}, + {"http://127.0.0.1:34601/pprof///heap///arg/", ERR_OK, "/pprof///heap///arg/"}, }; for (auto tt : tests) { @@ -38,7 +38,7 @@ TEST(http_server, parse_url) auto res = http_request::parse(m.get()); if (res.is_ok()) { - ASSERT_EQ(res.get_value().service_method, tt.result) << tt.url; + ASSERT_EQ(res.get_value().path, tt.path) << tt.url; } else { ASSERT_EQ(res.get_error().code(), tt.err); } @@ -48,9 +48,8 @@ TEST(http_server, parse_url) TEST(root_http_service_test, get_help) { http_server server(false); - auto root = new root_http_service(&server); + auto root = new root_http_service(); server.add_service(root); - ASSERT_EQ(server.get_help().size(), 1); http_request req; http_response resp; @@ -60,7 +59,6 @@ TEST(root_http_service_test, get_help) auto ver = new version_http_service(); server.add_service(ver); - ASSERT_EQ(server.get_help().size(), 2); root->default_handler(req, resp); ASSERT_EQ(resp.body, "{\"/\":\"ip:port/\",\"/version\":\"ip:port/version\"}\n"); } diff --git a/src/meta/meta_http_service.cpp b/src/meta/meta_http_service.cpp index e2de8cb433..4a470460c3 100644 --- a/src/meta/meta_http_service.cpp +++ b/src/meta/meta_http_service.cpp @@ -619,9 +619,7 @@ bool meta_http_service::redirect_if_not_primary(const http_request &req, http_re if (_service->_failure_detector->get_leader(&leader)) return true; // set redirect response - const std::string &service_name = req.service_method.first; - const std::string &method_name = req.service_method.second; - resp.location = "http://" + leader.to_std_string() + '/' + service_name + '/' + method_name; + resp.location = "http://" + leader.to_std_string() + '/' + req.path; if (!req.query_args.empty()) { resp.location += '?'; for (const auto &i : req.query_args) { From 6ecbf7d28a56fdb9dcaaf6ca78c5f58a9644f711 Mon Sep 17 00:00:00 2001 From: neverchanje Date: Thu, 20 Aug 2020 11:31:00 +0800 Subject: [PATCH 2/5] fix: make http_call_registry singleton --- src/http/http_call_registry.h | 10 +++++++++- src/http/http_server.cpp | 2 +- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/http/http_call_registry.h b/src/http/http_call_registry.h index 05b3588476..04e532fe89 100644 --- a/src/http/http_call_registry.h +++ b/src/http/http_call_registry.h @@ -42,9 +42,13 @@ class http_call_registry : public utils::singleton _call_map.erase(path); } - void add(std::shared_ptr call) + void add(std::unique_ptr call_uptr) { + auto call = std::shared_ptr(call_uptr.release()); std::lock_guard guard(_mu); + dassert(_call_map.find(call->path) == _call_map.end(), + "repeatedly register http call %s", + call->path.c_str()); _call_map[call->path] = call; } @@ -59,6 +63,10 @@ class http_call_registry : public utils::singleton return ret; } +private: + friend class utils::singleton; + http_call_registry() = default; + private: mutable std::mutex _mu; std::map> _call_map; diff --git a/src/http/http_server.cpp b/src/http/http_server.cpp index ed3831517c..92792f2f26 100644 --- a/src/http/http_server.cpp +++ b/src/http/http_server.cpp @@ -37,7 +37,7 @@ namespace dsn { void http_service::register_handler(std::string path, http_callback cb, std::string help) { - auto call = std::make_shared(); + auto call = make_unique(); call->path = this->path(); if (!path.empty()) { call->path += "/" + std::move(path); From a6d68e76fa6c4f6e5be713f0e0fd5f61dd23dae8 Mon Sep 17 00:00:00 2001 From: neverchanje Date: Thu, 20 Aug 2020 23:46:16 +0800 Subject: [PATCH 3/5] fix: add flag to skip http server --- include/dsn/http/http_server.h | 5 ++++- src/http/http_call_registry.h | 2 +- src/http/http_server.cpp | 9 +++++++-- src/http/test/http_server_test.cpp | 5 ++++- src/replica/storage/simple_kv/simple_kv.main.cpp | 3 +++ 5 files changed, 19 insertions(+), 5 deletions(-) diff --git a/include/dsn/http/http_server.h b/include/dsn/http/http_server.h index a1ceaff204..eb68f17340 100644 --- a/include/dsn/http/http_server.h +++ b/include/dsn/http/http_server.h @@ -8,9 +8,12 @@ #include #include #include +#include namespace dsn { +DSN_DECLARE_bool(enable_http_server); + enum http_method { HTTP_METHOD_GET = 1, @@ -73,7 +76,7 @@ class http_service class http_server : public serverlet { public: - explicit http_server(bool start = true); + explicit http_server(); ~http_server() override = default; diff --git a/src/http/http_call_registry.h b/src/http/http_call_registry.h index 04e532fe89..71ed659161 100644 --- a/src/http/http_call_registry.h +++ b/src/http/http_call_registry.h @@ -47,7 +47,7 @@ class http_call_registry : public utils::singleton auto call = std::shared_ptr(call_uptr.release()); std::lock_guard guard(_mu); dassert(_call_map.find(call->path) == _call_map.end(), - "repeatedly register http call %s", + "repeatedly register http call \"%s\"", call->path.c_str()); _call_map[call->path] = call; } diff --git a/src/http/http_server.cpp b/src/http/http_server.cpp index 92792f2f26..3560a08c3a 100644 --- a/src/http/http_server.cpp +++ b/src/http/http_server.cpp @@ -16,6 +16,8 @@ namespace dsn { +DSN_DEFINE_bool("http", enable_http_server, true, "whether to enable the embedded HTTP server"); + /*extern*/ std::string http_status_code_to_string(http_status_code code) { switch (code) { @@ -37,6 +39,9 @@ namespace dsn { void http_service::register_handler(std::string path, http_callback cb, std::string help) { + if (!FLAGS_enable_http_server) { + return; + } auto call = make_unique(); call->path = this->path(); if (!path.empty()) { @@ -47,9 +52,9 @@ void http_service::register_handler(std::string path, http_callback cb, std::str http_call_registry::instance().add(std::move(call)); } -http_server::http_server(bool start /*default=true*/) : serverlet("http_server") +http_server::http_server() : serverlet("http_server") { - if (!start) { + if (!FLAGS_enable_http_server) { return; } diff --git a/src/http/test/http_server_test.cpp b/src/http/test/http_server_test.cpp index 2429c7387a..f8c4ce056e 100644 --- a/src/http/test/http_server_test.cpp +++ b/src/http/test/http_server_test.cpp @@ -47,7 +47,9 @@ TEST(http_server, parse_url) TEST(root_http_service_test, get_help) { - http_server server(false); + bool old_flag_value = FLAGS_enable_http_server; + FLAGS_enable_http_server = false; + http_server server; auto root = new root_http_service(); server.add_service(root); @@ -61,6 +63,7 @@ TEST(root_http_service_test, get_help) server.add_service(ver); root->default_handler(req, resp); ASSERT_EQ(resp.body, "{\"/\":\"ip:port/\",\"/version\":\"ip:port/version\"}\n"); + FLAGS_enable_http_server = old_flag_value; // restore old value } class http_message_parser_test : public testing::Test diff --git a/src/replica/storage/simple_kv/simple_kv.main.cpp b/src/replica/storage/simple_kv/simple_kv.main.cpp index 3ef7cc505f..b5c2294de6 100644 --- a/src/replica/storage/simple_kv/simple_kv.main.cpp +++ b/src/replica/storage/simple_kv/simple_kv.main.cpp @@ -40,9 +40,12 @@ // framework specific tools #include #include +#include static void dsn_app_registration_simple_kv() { + dsn::FLAGS_enable_http_server = false; // disable http server + dsn::replication::application::simple_kv_service_impl::register_service(); dsn::service::meta_service_app::register_all(); From 94ccfa74a65c590f808c8d1afaffab61a4a1cab4 Mon Sep 17 00:00:00 2001 From: neverchanje Date: Fri, 21 Aug 2020 11:52:38 +0800 Subject: [PATCH 4/5] fix simplekv --- src/replica/storage/simple_kv/test/simple_kv.main.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/replica/storage/simple_kv/test/simple_kv.main.cpp b/src/replica/storage/simple_kv/test/simple_kv.main.cpp index f9c81d9893..4c562a8433 100644 --- a/src/replica/storage/simple_kv/test/simple_kv.main.cpp +++ b/src/replica/storage/simple_kv/test/simple_kv.main.cpp @@ -38,9 +38,11 @@ #include "case.h" #include "client.h" #include "simple_kv.server.impl.h" +#include void dsn_app_registration_simple_kv() { + dsn::FLAGS_enable_http_server = false; dsn::replication::test::simple_kv_service_impl::register_service(); dsn::service::meta_service_app::register_all(); From 22534c2f20c66c311749d985ab7d9b7eeadf96da Mon Sep 17 00:00:00 2001 From: neverchanje Date: Wed, 26 Aug 2020 17:02:09 +0800 Subject: [PATCH 5/5] fix build --- src/http/test/http_server_test.cpp | 21 +++++++++++---------- src/meta/test/meta_http_service_test.cpp | 4 ++-- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/http/test/http_server_test.cpp b/src/http/test/http_server_test.cpp index 2d8c550383..b05432605f 100644 --- a/src/http/test/http_server_test.cpp +++ b/src/http/test/http_server_test.cpp @@ -47,23 +47,24 @@ TEST(http_server, parse_url) TEST(root_http_service_test, get_help) { - bool old_flag_value = FLAGS_enable_http_server; - FLAGS_enable_http_server = false; - http_server server; - auto root = new root_http_service(); - server.add_service(root); + for (const auto &call : http_call_registry::instance().list_all_calls()) { + http_call_registry::instance().remove(call->path); + } + root_http_service root; http_request req; http_response resp; - root->default_handler(req, resp); + root.default_handler(req, resp); ASSERT_EQ(resp.status_code, http_status_code::ok); ASSERT_EQ(resp.body, "{\"/\":\"ip:port/\"}\n"); - auto ver = new version_http_service(); - server.add_service(ver); - root->default_handler(req, resp); + version_http_service ver; + root.default_handler(req, resp); ASSERT_EQ(resp.body, "{\"/\":\"ip:port/\",\"/version\":\"ip:port/version\"}\n"); - FLAGS_enable_http_server = old_flag_value; // restore old value + + for (const auto &call : http_call_registry::instance().list_all_calls()) { + http_call_registry::instance().remove(call->path); + } } class http_message_parser_test : public testing::Test diff --git a/src/meta/test/meta_http_service_test.cpp b/src/meta/test/meta_http_service_test.cpp index 55fa410cf1..103363ca53 100644 --- a/src/meta/test/meta_http_service_test.cpp +++ b/src/meta/test/meta_http_service_test.cpp @@ -20,6 +20,7 @@ class meta_http_service_test : public meta_test_base void SetUp() override { meta_test_base::SetUp(); + FLAGS_enable_http_server = false; // disable http server _mhs = dsn::make_unique(_ms.get()); create_app(test_app); } @@ -54,8 +55,7 @@ class meta_http_service_test : public meta_test_base _mhs->get_app_envs_handler(fake_req, fake_resp); // env (value_version, 1) was set by create_app - std::string fake_json = R"({")" + env_key + R"(":)" + - R"(")" + env_value + R"(",)" + + std::string fake_json = R"({")" + env_key + R"(":)" + R"(")" + env_value + R"(",)" + R"("value_version":"1"})" + "\n"; ASSERT_EQ(fake_resp.status_code, http_status_code::ok) << http_status_code_to_string(fake_resp.status_code);