This is an automated email from the ASF dual-hosted git repository. cmcfarlen pushed a commit to branch 10.0.x in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit bb0ab529f82936f9d04d52ba176dd4585e12291d Author: Leif Hedstrom <[email protected]> AuthorDate: Thu Jun 13 10:13:21 2024 -0600 Make Cripts contexts use Proxy Allocator, and cleanup (#11426) (cherry picked from commit 3f4fa598548675e652451ba34c8a28382e4f3298) --- CMakeLists.txt | 1 + .../cripts/cripts-connections.en.rst | 13 ++++ doc/developer-guide/cripts/cripts-misc.en.rst | 6 +- example/cripts/example1.cc | 14 ++-- include/cripts/Context.hpp | 84 ++++------------------ include/cripts/Crypto.hpp | 14 +++- include/cripts/Urls.hpp | 35 +++++++-- include/iocore/eventsystem/Thread.h | 3 + include/tscore/ink_config.h.cmake.in | 2 + src/cripts/Bundles/Common.cc | 1 + src/cripts/Context.cc | 67 +++++++++++++++++ src/cripts/Crypto.cc | 14 ---- src/cripts/Headers.cc | 14 ++++ src/cripts/Lulu.cc | 2 - src/cripts/Urls.cc | 7 ++ 15 files changed, 176 insertions(+), 101 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 5433cf6978..c13ca45af9 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -227,6 +227,7 @@ if(ENABLE_CRIPTS) # needed for cripts # however this might become a general requirement find_package(fmt 8.1 REQUIRED) + set(TS_HAS_CRIPTS TRUE) endif() find_package(Backtrace) diff --git a/doc/developer-guide/cripts/cripts-connections.en.rst b/doc/developer-guide/cripts/cripts-connections.en.rst index c10b9d4f34..d2bbde2fe4 100644 --- a/doc/developer-guide/cripts/cripts-connections.en.rst +++ b/doc/developer-guide/cripts/cripts-connections.en.rst @@ -73,6 +73,19 @@ Method Description ``socket()`` Returns the raw socket structure for the connection (use with care). ======================= ========================================================================= +The ``ip()`` and ``localIP()`` methods return the IP address as an object. In addition to the +automatic string conversion, it also has a special semantic string conversion which takes +IPv4 and IPv6 CIDR sizes. For example: + +.. code-block:: cpp + + do_remap() + { + borrow conn = Client::Connection::get(); + auto ip = conn.ip(); + + CDebug("Client IP CIDR: {}", ip.string(24, 64)); + .. _cripts-connections-variables: Connection Variables diff --git a/doc/developer-guide/cripts/cripts-misc.en.rst b/doc/developer-guide/cripts/cripts-misc.en.rst index ad4702c5cf..bffe53a82f 100644 --- a/doc/developer-guide/cripts/cripts-misc.en.rst +++ b/doc/developer-guide/cripts/cripts-misc.en.rst @@ -214,17 +214,17 @@ UUID ==== Cripts supports generating a few different UUID (Universally Unique Identifier), for -different purposes. The ``UUID`` object provides the following functions: +different purposes. The ``UUID`` class provides the following objects: ========================= ======================================================================= -Function Description +Object Description ========================= ======================================================================= ``UUID::Process`` Returns a UUID for the running process (changes on ATS startup). ``UUID::Unique`` Returns a completely unique UUID for the server and transacion. ``UUID::Request`` Returns a unique id for this request. ========================= ======================================================================= -Using the ``UUID`` object is simple, via the ``get()`` method. Here's an example: +Using the ``UUID`` object is simple, via the ``::get()`` method. Here's an example: .. code-block:: cpp diff --git a/example/cripts/example1.cc b/example/cripts/example1.cc index 4c2953573c..6b62f4faaa 100644 --- a/example/cripts/example1.cc +++ b/example/cripts/example1.cc @@ -53,6 +53,14 @@ do_txn_close() CDebug("Cool, TXN close also works"); } +do_cache_lookup() +{ + borrow url2 = Cache::URL::get(); + + CDebug("Cache URL: {}", url2.url()); + CDebug("Cache Host: {}", url2.host); +} + do_send_request() { borrow req = Server::Request::get(); @@ -110,10 +118,6 @@ do_send_response() resp.status = 222; } - borrow url2 = Cache::URL::get(); - - CDebug("Cache URL: {}", url2.url()); - CDebug("Cache Host: {}", url2.host); CDebug("Txn count: {}", conn.count()); } @@ -147,7 +151,7 @@ do_remap() CDebug("X-Miles = {}", req["X-Miles"]); CDebug("random(1000) = {}", Cript::random(1000)); - borrow url = Pristine::URL::get(); + borrow url = Client::URL::get(); auto old_port = url.port; CDebug("Method is {}", req.method); diff --git a/include/cripts/Context.hpp b/include/cripts/Context.hpp index 915569e5b1..07436f1d8c 100644 --- a/include/cripts/Context.hpp +++ b/include/cripts/Context.hpp @@ -23,9 +23,9 @@ #include "ts/ts.h" #include "cripts/Instance.hpp" - -// Some compile time options -#define USE_CONTEXT_POOL 1 +#include "cripts/Headers.hpp" +#include "cripts/Urls.hpp" +#include "cripts/Connections.hpp" // These are pretty arbitrary for now constexpr int CONTEXT_DATA_SLOTS = 4; @@ -38,66 +38,24 @@ class Context using DataType = std::variant<integer, double, boolean, void *, Cript::string>; public: - Context() = delete; - Context(const Context &) = delete; - void operator=(const Context &) = delete; - - // Freelist management, a thread_local freelist of Context objects. - static self_type * - factory(TSHttpTxn txn_ptr, TSHttpSsn ssn_ptr, TSRemapRequestInfo *rri_ptr, Cript::Instance &inst) - { -#if USE_CONTEXT_POOL - if (_contexts) { - auto tmp = _contexts; - - _contexts = _contexts->_next; - return new (tmp) self_type(txn_ptr, ssn_ptr, rri_ptr, inst); - } else { - return new self_type(txn_ptr, ssn_ptr, rri_ptr, inst); - } -#else - return new self_type(txn_ptr, ssn_ptr, rri_ptr, inst); -#endif - } + Context() = delete; + Context(const self_type &) = delete; + void operator=(const self_type &) = delete; - void - release() + // This will, and should, only be called via the ProxyAllocator as used in the factory. + Context(TSHttpTxn txn_ptr, TSHttpSsn ssn_ptr, TSRemapRequestInfo *rri_ptr, Cript::Instance &inst) : rri(rri_ptr), p_instance(inst) { -#if USE_CONTEXT_POOL - this->_next = _contexts; - _contexts = this; -#else - delete this; -#endif + state.txnp = txn_ptr; + state.ssnp = ssn_ptr; + state.context = this; } // Clear the cached header mloc's etc. - void - reset() - { - // Clear the initialized headers before calling next hook - if (_client_resp_header.initialized()) { - _client_resp_header.reset(); - } - if (_server_resp_header.initialized()) { - _server_resp_header.reset(); - } - if (_client_req_header.initialized()) { - _client_req_header.reset(); - } - if (_server_req_header.initialized()) { - _server_req_header.reset(); - } + void reset(); - // Clear the initialized URLs before calling next hook - // Note: The client_url doesn't need to be cleared, since it's from the RRI struct - if (_cache_url.initialized()) { - _cache_url.reset(); - } - if (_pristine_url.initialized()) { - _pristine_url.reset(); - } - } + // Freelist management, a thread_local freelist of Context objects. These are implemented in Lulu.cc. + static self_type *factory(TSHttpTxn txn_ptr, TSHttpSsn ssn_ptr, TSRemapRequestInfo *rri_ptr, Cript::Instance &inst); + void release(); // These fields are preserving the parameters as setup in DoRemap() Cript::Transaction state; @@ -109,14 +67,6 @@ public: // These are private, but needs to be visible to our friend classes that // depends on the Context. private: - // This can be private, since all constructors should go via the factory - Context(TSHttpTxn txn_ptr, TSHttpSsn ssn_ptr, TSRemapRequestInfo *rri_ptr, Cript::Instance &inst) : rri(rri_ptr), p_instance(inst) - { - state.txnp = txn_ptr; - state.ssnp = ssn_ptr; - state.context = this; - } - friend class Client::Request; friend class Client::Response; friend class Client::Connection; @@ -145,10 +95,6 @@ private: Server::Connection _server_conn; Cache::URL _cache_url; Parent::URL _parent_url; - - // For the thread_local freelist - static thread_local self_type *_contexts; - self_type *_next = nullptr; }; // End class Context } // namespace Cript diff --git a/include/cripts/Crypto.hpp b/include/cripts/Crypto.hpp index 1ca31d3f41..5ddfd9653c 100644 --- a/include/cripts/Crypto.hpp +++ b/include/cripts/Crypto.hpp @@ -25,6 +25,7 @@ #include <openssl/md5.h> #include <openssl/hmac.h> +#include "tsutil/StringConvert.h" #include "ts/ts.h" #include "ts/remap.h" @@ -71,7 +72,11 @@ namespace detail // ToDo: Not sure why, but some compilers says this is deprecated // void operator=(const Digest &) = delete; - [[nodiscard]] Cript::string hex() const; + [[nodiscard]] Cript::string + hex() const + { + return ts::hex({reinterpret_cast<const char *>(_hash), _length}); + } operator Cript::string() const { return hex(); } @@ -127,7 +132,12 @@ namespace detail return Crypto::Base64::encode(_message); } - [[nodiscard]] Cript::string hex() const; + [[nodiscard]] Cript::string + hex() const + { + return ts::hex(_message); + } + operator Cript::string() const { return hex(); } protected: diff --git a/include/cripts/Urls.hpp b/include/cripts/Urls.hpp index fbd4dbd3af..51f3ba42d0 100644 --- a/include/cripts/Urls.hpp +++ b/include/cripts/Urls.hpp @@ -28,6 +28,8 @@ #include "ts/remap.h" #include "ts/ts.h" +#include "cripts/Headers.hpp" + namespace Cript { class Context; @@ -550,6 +552,12 @@ public: return _urlp; } + virtual bool + readOnly() const + { + return false; + } + // Getters / setters for the full URL Cript::string url() const; @@ -567,13 +575,11 @@ protected: host._owner = path._owner = scheme._owner = query._owner = port._owner = this; } - TSMBuffer _bufp = nullptr; // These two gets setup via initializing, pointing - // to appropriate headers + TSMBuffer _bufp = nullptr; // These two gets setup via initializing, to appropriate headers TSMLoc _hdr_loc = nullptr; // Do not release any of this within the URL classes! TSMLoc _urlp = nullptr; // This is owned by us. Cript::Transaction *_state = nullptr; // Pointer into the owning Context's State - bool _modified = false; // We have pending changes on the path components or - // query parameters? + bool _modified = false; // We have pending changes on the path/query components }; // End class Url @@ -588,13 +594,18 @@ class URL : public Cript::Url using self_type = URL; public: - URL() = default; - + URL() = default; URL(const URL &) = delete; void operator=(const URL &) = delete; static URL &_get(Cript::Context *context); + bool + readOnly() const override + { + return true; + } + }; // End class Pristine::URL } // namespace Pristine @@ -647,6 +658,12 @@ namespace From { } + bool + readOnly() const override + { + return true; + } + static URL &_get(Cript::Context *context); bool _update(Cript::Context *context); @@ -675,6 +692,12 @@ namespace To { } + bool + readOnly() const override + { + return true; + } + static URL &_get(Cript::Context *context); bool _update(Cript::Context *context); diff --git a/include/iocore/eventsystem/Thread.h b/include/iocore/eventsystem/Thread.h index 31a446186b..22b006a496 100644 --- a/include/iocore/eventsystem/Thread.h +++ b/include/iocore/eventsystem/Thread.h @@ -140,6 +140,9 @@ public: ProxyAllocator INKContAllocator; ProxyAllocator INKVConnAllocator; ProxyAllocator mHandleAllocator; +#if TS_HAS_CRIPTS == 1 + ProxyAllocator criptContextAllocator; +#endif /** Start the underlying thread. diff --git a/include/tscore/ink_config.h.cmake.in b/include/tscore/ink_config.h.cmake.in index c9ac033886..b8f036457f 100644 --- a/include/tscore/ink_config.h.cmake.in +++ b/include/tscore/ink_config.h.cmake.in @@ -169,3 +169,5 @@ const int DEFAULT_STACKSIZE = @DEFAULT_STACK_SIZE@; #define TS_BUILD_CANONICAL_HOST "@CMAKE_HOST@" #cmakedefine YAMLCPP_LIB_VERSION "@YAMLCPP_LIB_VERSION@" + +#cmakedefine01 TS_HAS_CRIPTS diff --git a/src/cripts/Bundles/Common.cc b/src/cripts/Bundles/Common.cc index 96eb8dfcbb..cb9f663771 100644 --- a/src/cripts/Bundles/Common.cc +++ b/src/cripts/Bundles/Common.cc @@ -56,6 +56,7 @@ Common::doRemap(Cript::Context *context) // .dscp(int) if (_dscp > 0) { + CDebug("Setting DSCP = {}", _dscp); conn.dscp = _dscp; } } diff --git a/src/cripts/Context.cc b/src/cripts/Context.cc new file mode 100644 index 0000000000..c23a37c3ec --- /dev/null +++ b/src/cripts/Context.cc @@ -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. +*/ + +#include "iocore/eventsystem/EventSystem.h" +#undef Status // Major namespace conflict here with tscore/Diags.h::Status +#undef Error + +#include "cripts/Lulu.hpp" +#include "cripts/Context.hpp" + +void +Cript::Context::reset() +{ + // Clear the initialized headers before calling next hook + // Note: we don't clear the pristine URL, nor the Remap From/To URLs, they are static. + // We also don't clear the client URL, since it's from the RRI. + if (_client_resp_header.initialized()) { + _client_resp_header.reset(); + } + if (_server_resp_header.initialized()) { + _server_resp_header.reset(); + } + if (_client_req_header.initialized()) { + _client_req_header.reset(); + } + if (_server_req_header.initialized()) { + _server_req_header.reset(); + } + + // Clear the initialized URLs before calling next hook + if (_cache_url.initialized()) { + _cache_url.reset(); + } + if (_parent_url.initialized()) { + _parent_url.reset(); + } +} + +// Freelist management. These are here to avoid polluting the Context includes with ATS core includes. +ClassAllocator<Cript::Context> criptContextAllocator("Cript::Context"); + +Cript::Context * +Cript::Context::factory(TSHttpTxn txn_ptr, TSHttpSsn ssn_ptr, TSRemapRequestInfo *rri_ptr, Cript::Instance &inst) +{ + return THREAD_ALLOC(criptContextAllocator, this_thread(), txn_ptr, ssn_ptr, rri_ptr, inst); +} + +void +Cript::Context::release() +{ + THREAD_FREE(this, criptContextAllocator, this_thread()); +} diff --git a/src/cripts/Crypto.cc b/src/cripts/Crypto.cc index c3f11a60bb..8bf36080c2 100644 --- a/src/cripts/Crypto.cc +++ b/src/cripts/Crypto.cc @@ -20,7 +20,6 @@ #include "cripts/Lulu.hpp" #include "cripts/Preamble.hpp" -#include "tsutil/StringConvert.h" // From ATS, seems high ... #define ENCODED_LEN(len) (((int)ceil(1.34 * (len) + 5)) + 1) @@ -107,19 +106,6 @@ Crypto::Escape::decode(Cript::string_view str) return ret; // RVO } -// These may be small, but pulls in a fair amount of Boost code, so better keep them in the .cc -Cript::string -Crypto::detail::Digest::hex() const -{ - return ts::hex({reinterpret_cast<const char *>(_hash), _length}); -} - -Cript::string -Crypto::detail::Cipher::hex() const -{ - return ts::hex(_message); -} - Crypto::SHA256 Crypto::SHA256::encode(Cript::string_view str) { diff --git a/src/cripts/Headers.cc b/src/cripts/Headers.cc index 0e791b3e98..6f5b5509f8 100644 --- a/src/cripts/Headers.cc +++ b/src/cripts/Headers.cc @@ -318,6 +318,10 @@ Header::iterate() Client::Response & Client::Response::_get(Cript::Context *context) { + TSReleaseAssert(context->state.hook != TS_HTTP_READ_REQUEST_HDR_HOOK); + TSReleaseAssert(context->state.hook != TS_HTTP_POST_REMAP_HOOK); + TSReleaseAssert(context->state.hook != TS_HTTP_SEND_REQUEST_HDR_HOOK); + Client::Response *response = &context->_client_resp_header; if (!response->initialized()) { @@ -335,6 +339,11 @@ Client::Response::_get(Cript::Context *context) Server::Request & Server::Request::_get(Cript::Context *context) { + TSReleaseAssert(context->state.hook != TS_HTTP_READ_REQUEST_HDR_HOOK); + TSReleaseAssert(context->state.hook != TS_HTTP_POST_REMAP_HOOK); + TSReleaseAssert(context->state.hook != TS_HTTP_CACHE_LOOKUP_COMPLETE_HOOK); + TSReleaseAssert(context->state.hook != TS_HTTP_READ_RESPONSE_HDR_HOOK); + Server::Request *request = &context->_server_req_header; if (!request->initialized()) { @@ -352,6 +361,11 @@ Server::Request::_get(Cript::Context *context) Server::Response & Server::Response::_get(Cript::Context *context) { + TSReleaseAssert(context->state.hook != TS_HTTP_READ_REQUEST_HDR_HOOK); + TSReleaseAssert(context->state.hook != TS_HTTP_POST_REMAP_HOOK); + TSReleaseAssert(context->state.hook != TS_HTTP_CACHE_LOOKUP_COMPLETE_HOOK); + TSReleaseAssert(context->state.hook != TS_HTTP_SEND_REQUEST_HDR_HOOK); + Server::Response *response = &context->_server_resp_header; if (!response->initialized()) { diff --git a/src/cripts/Lulu.cc b/src/cripts/Lulu.cc index 6bdaefd47e..3d5e7afee9 100644 --- a/src/cripts/Lulu.cc +++ b/src/cripts/Lulu.cc @@ -188,5 +188,3 @@ std::string plugin_debug_tag; // This is to allow us to have a global initialization routine pthread_once_t init_once_control = PTHREAD_ONCE_INIT; pthread_once_t debug_tag_control = PTHREAD_ONCE_INIT; - -thread_local Cript::Context *Cript::Context::_contexts = nullptr; diff --git a/src/cripts/Urls.cc b/src/cripts/Urls.cc index 61066a6b30..70a66072b2 100644 --- a/src/cripts/Urls.cc +++ b/src/cripts/Urls.cc @@ -42,6 +42,7 @@ Cript::Url::Scheme::getSV() Cript::Url::Scheme Cript::Url::Scheme::operator=(Cript::string_view scheme) { + TSReleaseAssert(!_owner->readOnly()); // This can not be a read-only URL TSUrlSchemeSet(_owner->_bufp, _owner->_urlp, scheme.data(), scheme.size()); _owner->_modified = true; reset(); @@ -68,6 +69,7 @@ Cript::Url::Host::getSV() Cript::Url::Host Cript::Url::Host::operator=(Cript::string_view host) { + TSReleaseAssert(!_owner->readOnly()); // This can not be a read-only URL TSUrlHostSet(_owner->_bufp, _owner->_urlp, host.data(), host.size()); _owner->_modified = true; reset(); @@ -88,6 +90,7 @@ Cript::Url::Port::operator integer() // This should not be explicit Cript::Url::Port Cript::Url::Port::operator=(int port) { + TSReleaseAssert(!_owner->readOnly()); // This can not be a read-only URL TSUrlPortSet(_owner->_bufp, _owner->_urlp, port); _owner->_modified = true; reset(); @@ -138,6 +141,7 @@ Cript::Url::Path::operator[](Segments::size_type ix) Cript::Url::Path Cript::Url::Path::operator=(Cript::string_view path) { + TSReleaseAssert(!_owner->readOnly()); // This can not be a read-only URL TSUrlPathSet(_owner->_bufp, _owner->_urlp, path.data(), path.size()); _owner->_modified = true; reset(); @@ -163,6 +167,7 @@ Cript::Url::Path::operator+=(Cript::string_view add) Cript::Url::Path::String & Cript::Url::Path::String::operator=(const Cript::string_view str) { + TSReleaseAssert(!_owner->_owner->readOnly()); // This can not be a read-only URL _owner->_size -= _owner->_segments[_ix].size(); _owner->_segments[_ix] = str; _owner->_size += str.size(); @@ -209,6 +214,7 @@ Cript::Url::Path::_parser() Cript::Url::Query::Parameter & Cript::Url::Query::Parameter::operator=(const Cript::string_view str) { + TSReleaseAssert(!_owner->_owner->readOnly()); // This can not be a read-only URL auto iter = _owner->_hashed.find(_name); if (iter != _owner->_hashed.end()) { @@ -268,6 +274,7 @@ Cript::Url::Query::getSV() Cript::Url::Query Cript::Url::Query::operator=(Cript::string_view query) { + TSReleaseAssert(!_owner->readOnly()); // This can not be a read-only URL TSUrlHttpQuerySet(_owner->_bufp, _owner->_urlp, query.data(), query.size()); _owner->_modified = true; reset();
