diff --git a/include/proxy/ReverseProxy.h b/include/proxy/ReverseProxy.h index 201327d8aa2..de18033c8d5 100644 --- a/include/proxy/ReverseProxy.h +++ b/include/proxy/ReverseProxy.h @@ -43,12 +43,14 @@ #include "proxy/http/remap/UrlMapping.h" #include "mgmt/config/ConfigContext.h" +#include "tsutil/AtomicSharedPtr.h" + #define EMPTY_PORT_MAPPING (int32_t) ~0 class url_mapping; struct host_hdr_info; -extern std::atomic rewrite_table; +extern AtomicSharedPtr rewrite_table; // API Functions int init_reverse_proxy(); @@ -61,4 +63,10 @@ bool reloadUrlRewrite(ConfigContext ctx); bool urlRewriteVerify(); void init_remap_volume_host_records(); -int url_rewrite_CB(const char *name, RecDataT data_type, RecData data, void *cookie); + +// Synchronously drops rewrite_table. Call from a Continuation context +// before TSSystemState::shut_down_event_system() so plugin doneInstance() +// has this_ethread() for TSMutexLock. +void shutdown_url_rewrite(); + +int url_rewrite_CB(const char *name, RecDataT data_type, RecData data, void *cookie); diff --git a/include/proxy/http/HttpSM.h b/include/proxy/http/HttpSM.h index fc3e1252452..802fb94aa70 100644 --- a/include/proxy/http/HttpSM.h +++ b/include/proxy/http/HttpSM.h @@ -33,6 +33,7 @@ #include #include +#include #include "tscore/ink_platform.h" #include "iocore/eventsystem/EventSystem.h" @@ -306,7 +307,7 @@ class HttpSM : public Continuation, public PluginUserArgs // This unfortunately can't go into the t_state, because of circular dependencies. We could perhaps refactor // this, with a lot of work, but this is easier for now. - UrlRewrite *m_remap = nullptr; + std::shared_ptr m_remap; History history; NetVConnection * diff --git a/include/proxy/http/remap/UrlRewrite.h b/include/proxy/http/remap/UrlRewrite.h index cfe46817d02..e2c3604b554 100644 --- a/include/proxy/http/remap/UrlRewrite.h +++ b/include/proxy/http/remap/UrlRewrite.h @@ -24,7 +24,6 @@ #pragma once -#include "iocore/eventsystem/Freer.h" #include "mgmt/config/ConfigContext.h" #include "proxy/http/remap/UrlMapping.h" #include "proxy/http/remap/UrlMappingPathIndex.h" @@ -57,12 +56,12 @@ enum class mapping_type { /** * **/ -class UrlRewrite : public RefCountObjInHeap +class UrlRewrite { public: using URLTable = std::unordered_map; UrlRewrite() = default; - ~UrlRewrite() override; + ~UrlRewrite(); /** Retrieve the configured ACL matching policy. * @@ -93,26 +92,6 @@ class UrlRewrite : public RefCountObjInHeap void SetReverseFlag(int flag); void Print() const; - // The UrlRewrite object is-a RefCountObj, but this is a convenience to make it clear that we - // don't delete() these objects directly, but via the release() method only. - UrlRewrite * - acquire() - { - this->refcount_inc(); - return this; - } - - void - release() - { - if (0 == this->refcount_dec()) { - // Delete this on an ET_TASK thread, which avoids doing potentially slow things on an ET_NET thread. - static DbgCtl dc{"url_rewrite"}; - Dbg(dc, "Deleting old configuration immediately"); - new_Deleter(this, 0); - } - } - bool is_valid() const { diff --git a/include/tsutil/AtomicSharedPtr.h b/include/tsutil/AtomicSharedPtr.h new file mode 100644 index 00000000000..c7138a8e575 --- /dev/null +++ b/include/tsutil/AtomicSharedPtr.h @@ -0,0 +1,84 @@ +/** @file + + Atomic wrapper around std::shared_ptr with the C++20 + std::atomic> API. + + @section license License + + 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 + +// Use the C++20 std::atomic> specialization when the +// standard library provides it, otherwise fall back to the pre-C++20 +// std::atomic_*_explicit free-function overloads on shared_ptr. The +// fallback exists for libstdc++ < 12 and libc++ < 14, which predate the +// specialization. When those toolchains are no longer supported, delete +// the #else branch and the surrounding #if; call sites do not change. +#if defined(__cpp_lib_atomic_shared_ptr) && __cpp_lib_atomic_shared_ptr >= 201711L + +template using AtomicSharedPtr = std::atomic>; + +#else + +// Belt-and-suspenders: on the toolchains that take this branch (libstdc++ +// < 12, libc++ < 16) the free-function overloads are not yet marked +// [[deprecated]], so the suppression below is usually a no-op. It +// matters only if someone forces the fallback on a modern library (e.g. +// -D__cpp_lib_atomic_shared_ptr=0) or compiles against a library that +// ships the deprecation markers ahead of the specialization. +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wdeprecated-declarations" + +template class AtomicSharedPtr +{ +public: + AtomicSharedPtr() noexcept = default; + AtomicSharedPtr(std::shared_ptr desired) noexcept : _p(std::move(desired)) {} + + AtomicSharedPtr(const AtomicSharedPtr &) = delete; + AtomicSharedPtr &operator=(const AtomicSharedPtr &) = delete; + + std::shared_ptr + load(std::memory_order order = std::memory_order_seq_cst) const noexcept + { + return std::atomic_load_explicit(&_p, order); + } + + void + store(std::shared_ptr desired, std::memory_order order = std::memory_order_seq_cst) noexcept + { + std::atomic_store_explicit(&_p, std::move(desired), order); + } + + std::shared_ptr + exchange(std::shared_ptr desired, std::memory_order order = std::memory_order_seq_cst) noexcept + { + return std::atomic_exchange_explicit(&_p, std::move(desired), order); + } + +private: + std::shared_ptr _p; +}; + +#pragma GCC diagnostic pop + +#endif diff --git a/src/api/InkAPI.cc b/src/api/InkAPI.cc index 5500b44a6c8..0cfcdfa0f10 100644 --- a/src/api/InkAPI.cc +++ b/src/api/InkAPI.cc @@ -5068,7 +5068,7 @@ TSHttpTxnNextHopNamedStrategyGet(TSHttpTxn txnp, const char *name) auto sm = reinterpret_cast(txnp); - sdk_assert(sdk_sanity_check_null_ptr((void *)sm->m_remap) == TS_SUCCESS); + sdk_assert(sdk_sanity_check_null_ptr((void *)sm->m_remap.get()) == TS_SUCCESS); sdk_assert(sdk_sanity_check_null_ptr((void *)sm->m_remap->strategyFactory) == TS_SUCCESS); // HttpSM has a reference count handle to UrlRewrite which has a diff --git a/src/proxy/ReverseProxy.cc b/src/proxy/ReverseProxy.cc index 8368ad515e8..8600013624f 100644 --- a/src/proxy/ReverseProxy.cc +++ b/src/proxy/ReverseProxy.cc @@ -29,8 +29,10 @@ #include "tscore/ink_platform.h" #include "tscore/Filenames.h" +#include "tscore/TSSystemState.h" #include #include "iocore/cache/Cache.h" +#include "iocore/eventsystem/Freer.h" #include "proxy/ReverseProxy.h" #include "mgmt/config/ConfigContextDiags.h" #include "mgmt/config/ConfigRegistry.h" @@ -49,12 +51,44 @@ Ptr reconfig_mutex; DbgCtl dbg_ctl_url_rewrite{"url_rewrite"}; +// Steers UrlRewriteDeleter to inline-delete; see shutdown_url_rewrite(). +std::atomic rewrite_table_shutdown{false}; + +// Defer teardown to ET_TASK; UrlRewrite destruction can be slow. +struct UrlRewriteDeleter { + void + operator()(UrlRewrite *p) const noexcept + { + if (!p) { + return; + } + if (rewrite_table_shutdown.load(std::memory_order_acquire) || TSSystemState::is_event_system_shut_down()) { + // Leak; plugin teardown is unsafe post-shutdown. + return; + } + // new_Deleter allocates; fall back to inline delete so we don't escape noexcept. + try { + new_Deleter(p, 0); + } catch (...) { + delete p; + } + } +}; + } // end anonymous namespace // Global Ptrs -std::atomic rewrite_table = nullptr; +AtomicSharedPtr rewrite_table; thread_local PluginThreadContext *pluginThreadContext = nullptr; +void +shutdown_url_rewrite() +{ + // Drain before flag: this ref destructs normally; later drops leak. + rewrite_table.exchange(nullptr); + rewrite_table_shutdown.store(true, std::memory_order_release); +} + // Tokens for the Callback function #define FILE_CHANGED 0 #define REVERSE_CHANGED 1 @@ -71,10 +105,9 @@ static void init_table_volume_host_records(UrlRewrite &table); int init_reverse_proxy() { - ink_assert(rewrite_table.load() == nullptr); - reconfig_mutex = new_ProxyMutex(); - auto *initial_table = new UrlRewrite(); - initial_table->acquire(); + ink_assert(rewrite_table.load(std::memory_order_acquire) == nullptr); + reconfig_mutex = new_ProxyMutex(); + auto initial_table = std::make_unique(); // Register with ConfigRegistry BEFORE load() so that remap.config is in // FileManager's bindings when .include directives call configFileChild() @@ -100,7 +133,8 @@ init_reverse_proxy() init_table_volume_host_records(*initial_table); } - rewrite_table.store(initial_table, std::memory_order_release); + // Publish: shared_ptr semantics replace the prior bespoke acquire()/release() refcount on UrlRewrite. + rewrite_table.store(std::shared_ptr(initial_table.release(), UrlRewriteDeleter{}), std::memory_order_release); RecRegisterConfigUpdateCb("proxy.config.reverse_proxy.enabled", url_rewrite_CB, (void *)REVERSE_CHANGED); return 0; @@ -145,25 +179,19 @@ reloadUrlRewrite(ConfigContext ctx) std::string msg_buffer; msg_buffer.reserve(1024); - UrlRewrite *newTable, *oldTable; CfgLoadLog(ctx, DL_Note, "%s loading ...", ts::filename::REMAP); Dbg(dbg_ctl_url_rewrite, "%s updated, reloading...", ts::filename::REMAP); - newTable = new UrlRewrite(); + auto newTable = std::make_unique(); if (newTable->load(ctx)) { swoc::bwprint(msg_buffer, "{} finished loading", ts::filename::REMAP); - // Hold at least one lease, until we reload the configuration - newTable->acquire(); - - // Swap configurations - oldTable = rewrite_table.exchange(newTable); - - ink_assert(oldTable != nullptr); - - // Release the old one - oldTable->release(); + // Atomic publish: an old reader's shared_ptr keeps the prior table alive until its last + // ref is dropped; new readers see the new table. The prior race between load() and + // acquire() on the bespoke refcount cannot revive a table whose refcount was driven to + // zero, because there is no separate refcount. + rewrite_table.exchange(std::shared_ptr(newTable.release(), UrlRewriteDeleter{}), std::memory_order_acq_rel); Dbg(dbg_ctl_url_rewrite, "%s", msg_buffer.c_str()); CfgLoadComplete(ctx, "%s finished loading", ts::filename::REMAP); @@ -171,7 +199,7 @@ reloadUrlRewrite(ConfigContext ctx) } else { swoc::bwprint(msg_buffer, "{} failed to load", ts::filename::REMAP); - delete newTable; + // newTable is a unique_ptr; falling out of scope deletes it. Dbg(dbg_ctl_url_rewrite, "%s", msg_buffer.c_str()); CfgLoadFail(ctx, "%s failed to load", ts::filename::REMAP); return false; @@ -233,25 +261,23 @@ init_remap_volume_host_records() return; } - UrlRewrite *table = rewrite_table.load(std::memory_order_acquire); + auto table = rewrite_table.load(std::memory_order_acquire); if (!table) { return; } - table->acquire(); - if (table->is_valid()) { init_table_volume_host_records(*table); } - - table->release(); } int url_rewrite_CB(const char * /* name ATS_UNUSED */, RecDataT /* data_type ATS_UNUSED */, RecData data, void * /* cookie ATS_UNUSED */) { - rewrite_table.load()->SetReverseFlag(data.rec_int); + if (auto table = rewrite_table.load(std::memory_order_acquire); table != nullptr) { + table->SetReverseFlag(data.rec_int); + } return 0; } diff --git a/src/proxy/http/HttpSM.cc b/src/proxy/http/HttpSM.cc index 9a623ffa9e4..4a07eef4b79 100644 --- a/src/proxy/http/HttpSM.cc +++ b/src/proxy/http/HttpSM.cc @@ -285,13 +285,7 @@ HttpSM::~HttpSM() // coverity[exn_spec_violation] - release() only does ref counting and delete on POD types HttpConfig::release(t_state.http_config_param); - - // m_remap->release() can allocate (new_Deleter), so catch potential bad_alloc - try { - m_remap->release(); - } catch (...) { - Error("Exception in ~HttpSM during m_remap->release"); - } + m_remap.reset(); // coverity[exn_spec_violation] - cancel_pending_action() cancels pending cache work and clears tracked pointers cache_sm.cancel_pending_action(); @@ -334,8 +328,9 @@ HttpSM::init(bool from_early_data) t_state.state_machine = this; t_state.http_config_param = HttpConfig::acquire(); - // Acquire a lease on the global remap / rewrite table (stupid global name ...) - m_remap = rewrite_table.load()->acquire(); + // Snapshot the global remap / rewrite table. shared_ptr keeps it alive across the txn + // even if reload swaps the global pointer concurrently. + m_remap = rewrite_table.load(std::memory_order_acquire); // Simply point to the global config for the time being, no need to copy this // entire struct if nothing is going to change it. @@ -4517,7 +4512,7 @@ HttpSM::state_remap_request(int event, void * /* data ATS_UNUSED */) case EVENT_REMAP_COMPLETE: { pending_action = nullptr; SMDbg(dbg_ctl_url_rewrite, "completed processor-based remapping request"); - t_state.url_remap_success = remapProcessor.finish_remap(&t_state, m_remap); + t_state.url_remap_success = remapProcessor.finish_remap(&t_state, m_remap.get()); call_transact_and_set_next_state(nullptr); break; } @@ -4594,7 +4589,7 @@ HttpSM::do_remap_request(bool run_inline) { SMDbg(dbg_ctl_http_seq, "Remapping request"); SMDbg(dbg_ctl_url_rewrite, "Starting a possible remapping for request"); - bool ret = remapProcessor.setup_for_remap(&t_state, m_remap); + bool ret = remapProcessor.setup_for_remap(&t_state, m_remap.get()); check_sni_host(); @@ -8182,7 +8177,7 @@ HttpSM::set_next_state() case HttpTransact::StateMachineAction_t::REMAP_REQUEST: { do_remap_request(true); /* run inline */ SMDbg(dbg_ctl_url_rewrite, "completed inline remapping request"); - t_state.url_remap_success = remapProcessor.finish_remap(&t_state, m_remap); + t_state.url_remap_success = remapProcessor.finish_remap(&t_state, m_remap.get()); if (t_state.next_action == HttpTransact::StateMachineAction_t::SEND_ERROR_CACHE_NOOP && t_state.transact_return_point == nullptr) { // It appears that we can now set the next_action to error and transact_return_point to nullptr when diff --git a/src/proxy/http/HttpTransact.cc b/src/proxy/http/HttpTransact.cc index 8180f845a22..5c0bf0f5172 100644 --- a/src/proxy/http/HttpTransact.cc +++ b/src/proxy/http/HttpTransact.cc @@ -1534,7 +1534,7 @@ HttpTransact::handleIfRedirect(State *s) mapping_type answer; URL redirect_url; - answer = request_url_remap_redirect(&s->hdr_info.client_request, &redirect_url, s->state_machine->m_remap); + answer = request_url_remap_redirect(&s->hdr_info.client_request, &redirect_url, s->state_machine->m_remap.get()); if ((answer == mapping_type::PERMANENT_REDIRECT) || (answer == mapping_type::TEMPORARY_REDIRECT)) { s->remap_redirect = redirect_url.string_get_ref(nullptr); if (answer == mapping_type::TEMPORARY_REDIRECT) { @@ -8239,7 +8239,7 @@ HttpTransact::build_response(State *s, HTTPHdr *base_response, HTTPHdr *outgoing // process reverse mappings on the location header // TS-1364: do this regardless of response code - response_url_remap(outgoing_response, s->state_machine->m_remap); + response_url_remap(outgoing_response, s->state_machine->m_remap.get()); if (s->http_config_param->enable_http_stats) { HttpTransactHeaders::generate_and_set_squid_codes(outgoing_response, s->via_string, &s->squid_codes); diff --git a/src/proxy/http/remap/RemapProcessor.cc b/src/proxy/http/remap/RemapProcessor.cc index 550af9360e6..22cfddff932 100644 --- a/src/proxy/http/remap/RemapProcessor.cc +++ b/src/proxy/http/remap/RemapProcessor.cc @@ -50,8 +50,13 @@ RemapProcessor::setup_for_remap(HttpTransact::State *s, UrlRewrite *table) int request_port; bool proxy_request = false; - s->reverse_proxy = table->reverse_proxy; s->url_map.set(s->hdr_info.client_request.m_heap); + if (unlikely(table == nullptr)) { + Dbg(dbg_ctl_url_rewrite, "no remap table (shutdown in progress); skipping remap"); + return false; + } + + s->reverse_proxy = table->reverse_proxy; ink_assert(redirect_url != nullptr); @@ -158,13 +163,20 @@ RemapProcessor::finish_remap(HttpTransact::State *s, UrlRewrite *table) { url_mapping *map = nullptr; HTTPHdr *request_header = &s->hdr_info.client_request; - URL *request_url = request_header->url_get(); + URL *request_url = nullptr; char **redirect_url = &s->remap_redirect; char tmp_referer_buf[4096], tmp_redirect_buf[4096], tmp_buf[2048]; int tmp; int from_len; referer_info *ri; + if (unlikely(table == nullptr)) { + Dbg(dbg_ctl_url_rewrite, "no remap table (shutdown in progress); skipping remap completion"); + return false; + } + + request_url = request_header->url_get(); + map = s->url_map.getMapping(); if (nullptr == map) { Dbg(dbg_ctl_url_rewrite, "Could not find corresponding url_mapping for this transaction"); diff --git a/src/proxy/http/unit_tests/test_HttpTransact.cc b/src/proxy/http/unit_tests/test_HttpTransact.cc index af283fb6017..ce4c07e0f81 100644 --- a/src/proxy/http/unit_tests/test_HttpTransact.cc +++ b/src/proxy/http/unit_tests/test_HttpTransact.cc @@ -31,6 +31,7 @@ using namespace std::string_view_literals; #include "tsutil/PostScript.h" #include "proxy/http/HttpTransact.h" +#include "proxy/http/remap/RemapProcessor.h" #include "records/RecordsConfig.h" #include @@ -41,6 +42,15 @@ TEST_CASE("HttpTransact", "[http]") mime_init(); http_init(); + SECTION("RemapProcessor tolerates a missing remap table") + { + HttpTransact::State state; + RemapProcessor processor; + + CHECK_FALSE(processor.setup_for_remap(&state, nullptr)); + CHECK_FALSE(processor.finish_remap(&state, nullptr)); + } + SECTION("HttpTransact::merge_response_header_with_cached_header") { SECTION("Basic") diff --git a/src/traffic_server/traffic_server.cc b/src/traffic_server/traffic_server.cc index 241c3850ca3..76e9c62cd95 100644 --- a/src/traffic_server/traffic_server.cc +++ b/src/traffic_server/traffic_server.cc @@ -92,6 +92,7 @@ extern "C" int plock(int); #include "records/RecordsConfig.h" #include "iocore/eventsystem/RecProcess.h" #include "proxy/Transform.h" +#include "proxy/ReverseProxy.h" #include "iocore/eventsystem/ConfigProcessor.h" #include "mgmt/config/ConfigContextDiags.h" #include "mgmt/config/ConfigRegistry.h" @@ -296,6 +297,8 @@ struct AutoStopCont : public Continuation { // Push buffered log entries into the preproc queue before shutdown. Log::flush_all_objects(); + shutdown_url_rewrite(); + TSSystemState::shut_down_event_system(); // Wake preproc threads to drain remaining log buffers before exit.