This is an automated email from the ASF dual-hosted git repository.
zwoop pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 3f4fa59854 Make Cripts contexts use Proxy Allocator, and cleanup
(#11426)
3f4fa59854 is described below
commit 3f4fa598548675e652451ba34c8a28382e4f3298
Author: Leif Hedstrom <[email protected]>
AuthorDate: Thu Jun 13 10:13:21 2024 -0600
Make Cripts contexts use Proxy Allocator, and cleanup (#11426)
---
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 b4f2e19523..25b8e357b6 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -228,6 +228,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 3b14278b7b..614eb220f4 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 d923ab152d..085731f6d8 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();
