You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by zw...@apache.org on 2020/06/04 01:41:09 UTC
[trafficserver] branch 9.0.x updated: Removes refcounting from
compress and s3_auth plugins
This is an automated email from the ASF dual-hosted git repository.
zwoop pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/9.0.x by this push:
new d26ff2b Removes refcounting from compress and s3_auth plugins
d26ff2b is described below
commit d26ff2b2ec98135f32c75372c4da36e016c96bb9
Author: Randall Meyer <rr...@apache.org>
AuthorDate: Wed Apr 29 07:55:34 2020 -0700
Removes refcounting from compress and s3_auth plugins
refcounting added at the remap level in 12d305b8d465710e6fc7a036287f1fea733c22c0
(cherry picked from commit 5005ce53a3ef6ab818392f967c240a391fcf7add)
---
plugins/background_fetch/configs.h | 2 +-
plugins/compress/compress.cc | 12 ++--------
plugins/compress/configuration.cc | 12 ++--------
plugins/compress/configuration.h | 24 +++-----------------
plugins/experimental/geoip_acl/lulu.h | 1 -
plugins/header_rewrite/header_rewrite.cc | 2 --
plugins/s3_auth/s3_auth.cc | 38 +++-----------------------------
7 files changed, 11 insertions(+), 80 deletions(-)
diff --git a/plugins/background_fetch/configs.h b/plugins/background_fetch/configs.h
index ba6dd35..0517ffa 100644
--- a/plugins/background_fetch/configs.h
+++ b/plugins/background_fetch/configs.h
@@ -34,7 +34,7 @@
const char PLUGIN_NAME[] = "background_fetch";
///////////////////////////////////////////////////////////////////////////
-// This holds one complete background fetch rule, which is also ref-counted.
+// This holds one complete background fetch rule
//
class BgFetchConfig
{
diff --git a/plugins/compress/compress.cc b/plugins/compress/compress.cc
index c6c560b..e6ac4e6 100644
--- a/plugins/compress/compress.cc
+++ b/plugins/compress/compress.cc
@@ -873,7 +873,6 @@ transform_plugin(TSCont contp, TSEvent event, void *edata)
case TS_EVENT_HTTP_TXN_CLOSE:
// Release the ocnif lease, and destroy this continuation
- hc->release();
TSContDestroy(contp);
break;
@@ -904,9 +903,9 @@ handle_request(TSHttpTxn txnp, Configuration *config)
if (TSHttpTxnClientReqGet(txnp, &req_buf, &req_loc) == TS_SUCCESS) {
if (config == nullptr) {
- hc = find_host_configuration(txnp, req_buf, req_loc, nullptr); // Get a lease on the global config
+ hc = find_host_configuration(txnp, req_buf, req_loc, nullptr);
} else {
- hc = find_host_configuration(txnp, req_buf, req_loc, config); // Get a lease on the local config passed through doRemap
+ hc = find_host_configuration(txnp, req_buf, req_loc, config);
}
bool allowed = false;
@@ -929,8 +928,6 @@ handle_request(TSHttpTxn txnp, Configuration *config)
normalize_accept_encoding(txnp, req_buf, req_loc);
TSHttpTxnHookAdd(txnp, TS_HTTP_CACHE_LOOKUP_COMPLETE_HOOK, transform_contp);
TSHttpTxnHookAdd(txnp, TS_HTTP_TXN_CLOSE_HOOK, transform_contp); // To release the config
- } else {
- hc->release(); // No longer need this configuration, release it.
}
TSHandleMLocRelease(req_buf, TS_NULL_MLOC, req_loc);
}
@@ -965,11 +962,7 @@ load_global_configuration(TSCont contp)
debug("config swapped, old config %p", oldconfig);
- // First, if there was a previous configuration, clean that one out. This avoids the
- // small race condition tht exist between doing a find() and calling hold() on a
- // HostConfig object.
if (prev_config) {
- prev_config->release_all();
debug("deleting previous configuration container, %p", prev_config);
delete prev_config;
}
@@ -1072,7 +1065,6 @@ TSRemapDeleteInstance(void *instance)
{
debug("Cleanup configs read from remap");
auto c = static_cast<Configuration *>(instance);
- c->release_all();
delete c;
}
diff --git a/plugins/compress/configuration.cc b/plugins/compress/configuration.cc
index 67ced83..e89a494 100644
--- a/plugins/compress/configuration.cc
+++ b/plugins/compress/configuration.cc
@@ -27,6 +27,8 @@
#include <vector>
#include <fnmatch.h>
+#include "debug_macros.h"
+
namespace Gzip
{
using namespace std;
@@ -110,7 +112,6 @@ enum ParserState {
void
Configuration::add_host_configuration(HostConfiguration *hc)
{
- hc->hold(); // We hold a lease on the HostConfig while it's in this container
host_configurations_.push_back(hc);
}
@@ -152,18 +153,9 @@ Configuration::find(const char *host, int host_length)
}
}
- host_configuration->hold(); // Hold a lease
return host_configuration;
}
-void
-Configuration::release_all()
-{
- for (auto &host_configuration : host_configurations_) {
- host_configuration->release();
- }
-}
-
bool
HostConfiguration::is_url_allowed(const char *url, int url_len)
{
diff --git a/plugins/compress/configuration.h b/plugins/compress/configuration.h
index df5f74d..67e81ff 100644
--- a/plugins/compress/configuration.h
+++ b/plugins/compress/configuration.h
@@ -26,8 +26,8 @@
#include <set>
#include <string>
#include <vector>
-#include "debug_macros.h"
-#include "tscore/ink_atomic.h"
+
+#include "ts/ts.h"
#include "tscpp/api/noncopyable.h"
namespace Gzip
@@ -51,8 +51,7 @@ public:
remove_accept_encoding_(false),
flush_(false),
compression_algorithms_(ALGORITHM_GZIP),
- minimum_content_length_(1024),
- ref_count_(0)
+ minimum_content_length_(1024)
{
}
@@ -128,21 +127,6 @@ public:
void add_compression_algorithms(std::string &algorithms);
int compression_algorithms();
- // Ref-counting these host configuration objects
- void
- hold()
- {
- ink_atomic_increment(&ref_count_, 1);
- }
- void
- release()
- {
- if (1 >= ink_atomic_decrement(&ref_count_, 1)) {
- debug("released and deleting HostConfiguration for %s settings", host_.size() > 0 ? host_.c_str() : "global");
- delete this;
- }
- }
-
private:
std::string host_;
bool enabled_;
@@ -151,7 +135,6 @@ private:
bool flush_;
int compression_algorithms_;
unsigned int minimum_content_length_;
- int ref_count_;
StringContainer compressible_content_types_;
StringContainer allows_;
@@ -169,7 +152,6 @@ class Configuration : private atscppapi::noncopyable
public:
static Configuration *Parse(const char *path);
HostConfiguration *find(const char *host, int host_length);
- void release_all();
private:
explicit Configuration() {}
diff --git a/plugins/experimental/geoip_acl/lulu.h b/plugins/experimental/geoip_acl/lulu.h
index c4c9cd9..b76ce1a 100644
--- a/plugins/experimental/geoip_acl/lulu.h
+++ b/plugins/experimental/geoip_acl/lulu.h
@@ -25,7 +25,6 @@
#include <sys/types.h>
#include "tscore/ink_defs.h"
-#include "tscore/ink_atomic.h"
// Used for Debug etc.
static const char *PLUGIN_NAME = "geoip_acl";
diff --git a/plugins/header_rewrite/header_rewrite.cc b/plugins/header_rewrite/header_rewrite.cc
index b728e42..a0e6707 100644
--- a/plugins/header_rewrite/header_rewrite.cc
+++ b/plugins/header_rewrite/header_rewrite.cc
@@ -22,8 +22,6 @@
#include "ts/ts.h"
#include "ts/remap.h"
-#include "tscore/ink_atomic.h"
-
#include "parser.h"
#include "ruleset.h"
#include "resources.h"
diff --git a/plugins/s3_auth/s3_auth.cc b/plugins/s3_auth/s3_auth.cc
index 8352195..4261406 100644
--- a/plugins/s3_auth/s3_auth.cc
+++ b/plugins/s3_auth/s3_auth.cc
@@ -41,8 +41,6 @@
#include <ts/remap.h>
#include "tscore/ink_config.h"
-// Special snowflake here, only available when building inside the ATS source tree.
-#include "tscore/ink_atomic.h"
#include "aws_auth_v4.h"
///////////////////////////////////////////////////////////////////////////////
@@ -208,22 +206,6 @@ public:
return true;
}
- void
- acquire()
- {
- ink_atomic_increment(&_ref_count, 1);
- }
-
- void
- release()
- {
- TSDebug(PLUGIN_NAME, "ref_count is %d", _ref_count);
- if (1 >= ink_atomic_decrement(&_ref_count, 1)) {
- TSDebug(PLUGIN_NAME, "configuration deleted, due to ref-counting");
- delete this;
- }
- }
-
// Used to copy relevant configurations that can be configured in a config file. Note: we intentionally
// don't override/use the assignment operator, since we only copy things IF they have been modified.
void
@@ -407,7 +389,6 @@ public:
schedule(TSHttpTxn txnp) const
{
TSHttpTxnHookAdd(txnp, TS_HTTP_SEND_REQUEST_HDR_HOOK, _cont);
- TSHttpTxnHookAdd(txnp, TS_HTTP_TXN_CLOSE_HOOK, _cont); // To release the config lease
}
private:
@@ -422,7 +403,6 @@ private:
bool _version_modified = false;
bool _virt_host_modified = false;
TSCont _cont = nullptr;
- int _ref_count = 1;
StringSet _v4includeHeaders;
bool _v4includeHeaders_modified = false;
StringSet _v4excludeHeaders;
@@ -522,15 +502,11 @@ ConfigCache::get(const char *fname)
TSDebug(PLUGIN_NAME, "Configuration from %s is stale, reloading", config_fname.c_str());
it->second.second = tv.tv_sec;
- if (nullptr != it->second.first) {
- // The previous config update / reload attempt did not fail, safe to call release.
- it->second.first->release();
- }
if (s3->parse_config(config_fname)) {
it->second.first = s3;
} else {
// Failed the configuration parse... Set the cache response to nullptr
- s3->release();
+ delete s3;
it->second.first = nullptr;
}
} else {
@@ -545,7 +521,7 @@ ConfigCache::get(const char *fname)
_cache[config_fname] = std::make_pair(s3, tv.tv_sec);
TSDebug(PLUGIN_NAME, "Parsing and caching configuration from %s, version:%d", config_fname.c_str(), s3->version());
} else {
- s3->release();
+ delete s3;
return nullptr;
}
@@ -911,9 +887,6 @@ event_handler(TSCont cont, TSEvent event, void *edata)
enable_event = TS_EVENT_HTTP_ERROR;
}
break;
- case TS_EVENT_HTTP_TXN_CLOSE:
- s3->release(); // Release the configuration lease when txn closes
- break;
default:
TSError("[%s] Unknown event for this plugin", PLUGIN_NAME);
TSDebug(PLUGIN_NAME, "unknown event for this plugin");
@@ -981,7 +954,6 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSE
if (!file_config) {
TSError("[%s] invalid configuration file, %s", PLUGIN_NAME, optarg);
*ih = nullptr;
- s3->release();
return TS_ERROR;
}
break;
@@ -1024,12 +996,10 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSE
// Make sure we got both the shared secret and the AWS secret
if (!s3->valid()) {
TSError("[%s] requires both shared and AWS secret configuration", PLUGIN_NAME);
- s3->release();
*ih = nullptr;
return TS_ERROR;
}
- // Note that we don't acquire() the s3 config, it's implicit that we hold at least one ref
*ih = static_cast<void *>(s3);
TSDebug(PLUGIN_NAME, "New rule: access_key=%s, virtual_host=%s, version=%d", s3->keyid(), s3->virt_host() ? "yes" : "no",
s3->version());
@@ -1041,8 +1011,7 @@ void
TSRemapDeleteInstance(void *ih)
{
S3Config *s3 = static_cast<S3Config *>(ih);
-
- s3->release();
+ delete s3;
}
///////////////////////////////////////////////////////////////////////////////
@@ -1055,7 +1024,6 @@ TSRemapDoRemap(void *ih, TSHttpTxn txnp, TSRemapRequestInfo * /* rri */)
if (s3) {
TSAssert(s3->valid());
- s3->acquire(); // Increase ref-count
// Now schedule the continuation to update the URL when going to origin.
// Note that in most cases, this is a No-Op, assuming you have reasonable
// cache hit ratio. However, the scheduling is next to free (very cheap).