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 2019/01/28 15:28:31 UTC
[trafficserver] branch master updated: Various fixes and
improvements to background_fetch
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 140e3a3 Various fixes and improvements to background_fetch
140e3a3 is described below
commit 140e3a3a49a85911de5d6b24fc2d39c5e77367ff
Author: Leif Hedstrom <zw...@apache.org>
AuthorDate: Wed Jan 23 14:31:25 2019 -0700
Various fixes and improvements to background_fetch
These changes include:
1) Adds a new option, --allow-304 / -a, which allows 304 responses
to still kick off a background fetch. This is important for request
that comes in with e.g.
Range: bytes=0-12
If-Modified-Since: Tue, 22 Jan 2019 19:36:03 GMT
2) Similar, to make the conditional request succeed properly with
conditional requests, we have to filter out more headers. In addition
to the Range: header, we now also filter out any values of
If-Match
If-Modified-Since
If-None-Match
If-Range
If-Unmodified-Since
3) To make all of this work with both global and per-remap setups,
we generalize the option parsing to work equally for both types. To
be backward compatible, a single option without a leading '-' is
assumed to be the name of the configuration file, but the correct
way to use this in a remap plugin is
@pparam=--config=/some/config/file.conf
4) In addition, while reading up on this, we should allow the
background fetch to be kicked off with an If-Range header.
---
plugins/background_fetch/background_fetch.cc | 122 ++++++++++++++++++---------
plugins/background_fetch/configs.cc | 51 ++++++++++-
plugins/background_fetch/configs.h | 21 ++++-
3 files changed, 148 insertions(+), 46 deletions(-)
diff --git a/plugins/background_fetch/background_fetch.cc b/plugins/background_fetch/background_fetch.cc
index 394a3dc..f6b99c9 100644
--- a/plugins/background_fetch/background_fetch.cc
+++ b/plugins/background_fetch/background_fetch.cc
@@ -25,11 +25,12 @@
#include <cstdio>
#include <cstring>
#include <cstdarg>
-#include <getopt.h>
#include <string>
#include <unordered_map>
#include <cinttypes>
+#include <string_view>
+#include <array>
#include "ts/ts.h"
#include "ts/remap.h"
@@ -38,12 +39,22 @@
#include "configs.h"
// Global config, if we don't have a remap specific config.
-static BgFetchConfig *gConfig;
+static BgFetchConfig *gConfig = nullptr;
+
+// This is the list of all headers that must be removed when we make the actual background
+// fetch request.
+static const std::array<const std::string_view, 6> FILTER_HEADERS{
+ {{TS_MIME_FIELD_RANGE, static_cast<size_t>(TS_MIME_LEN_RANGE)},
+ {TS_MIME_FIELD_IF_MATCH, static_cast<size_t>(TS_MIME_LEN_IF_MATCH)},
+ {TS_MIME_FIELD_IF_MODIFIED_SINCE, static_cast<size_t>(TS_MIME_LEN_IF_MODIFIED_SINCE)},
+ {TS_MIME_FIELD_IF_NONE_MATCH, static_cast<size_t>(TS_MIME_LEN_IF_NONE_MATCH)},
+ {TS_MIME_FIELD_IF_RANGE, static_cast<size_t>(TS_MIME_LEN_IF_RANGE)},
+ {TS_MIME_FIELD_IF_UNMODIFIED_SINCE, static_cast<size_t>(TS_MIME_LEN_IF_UNMODIFIED_SINCE)}}};
///////////////////////////////////////////////////////////////////////////
-// Hold the global ackground fetch state. This is currently shared across all
+// Hold the global background fetch state. This is currently shared across all
// configurations, as a singleton. ToDo: Would it ever make sense to do this
-// per remap rule? Probably not.
+// per remap rule? Maybe for per-remap logging ??
typedef std::unordered_map<std::string, bool> OutstandingRequests;
class BgFetchState
@@ -61,11 +72,16 @@ public:
}
~BgFetchState() { TSMutexDestroy(_lock); }
+
void
- createLog(const char *log_name)
+ createLog(const std::string &log_name)
{
- TSDebug(PLUGIN_NAME, "Creating log name %s", log_name);
- TSAssert(TS_SUCCESS == TSTextLogObjectCreate(log_name, TS_LOG_MODE_ADD_TIMESTAMP, &_log));
+ if (!_log) {
+ TSDebug(PLUGIN_NAME, "Creating log name %s", log_name.c_str());
+ TSAssert(TS_SUCCESS == TSTextLogObjectCreate(log_name.c_str(), TS_LOG_MODE_ADD_TIMESTAMP, &_log));
+ } else {
+ TSError("[%s] A log file was already create, ignoring creation of %s", PLUGIN_NAME, log_name.c_str());
+ }
}
TSTextLogObject
@@ -261,10 +277,13 @@ BgFetchData::initialize(TSMBuffer request, TSMLoc req_hdr, TSHttpTxn txnp)
TSDebug(PLUGIN_NAME, "Set header Host: %.*s", len, hostp);
}
- // Next, remove any Range: headers from our request.
- if (remove_header(mbuf, hdr_loc, TS_MIME_FIELD_RANGE, TS_MIME_LEN_RANGE) > 0) {
- TSDebug(PLUGIN_NAME, "Removed the Range: header from request");
+ // Next, remove the Range headers and IMS (conditional) headers from the request
+ for (auto const &header : FILTER_HEADERS) {
+ if (remove_header(mbuf, hdr_loc, header.data(), header.size()) > 0) {
+ TSDebug(PLUGIN_NAME, "Removed the %s header from request", header.data());
+ }
}
+
// Everything went as planned, so we can return true
ret = true;
}
@@ -507,9 +526,10 @@ cont_handle_response(TSCont contp, TSEvent event, void *edata)
// ToDo: Check the MIME type first, to see if it's a type we care about.
// ToDo: Such MIME types should probably be per remap rule.
- // Only deal with 206 responses from Origin
- TSDebug(PLUGIN_NAME, "Testing: response is 206?");
- if (TS_HTTP_STATUS_PARTIAL_CONTENT == TSHttpHdrStatusGet(response, resp_hdr)) {
+ // Only deal with 206 and possibly 304 responses from Origin
+ TSHttpStatus status = TSHttpHdrStatusGet(response, resp_hdr);
+ TSDebug(PLUGIN_NAME, "Testing: response status code: %d?", status);
+ if (TS_HTTP_STATUS_PARTIAL_CONTENT == status || (config->allow304() && TS_HTTP_STATUS_NOT_MODIFIED == status)) {
// Everything looks good so far, add a TXN hook for SEND_RESPONSE_HDR
TSCont contp = TSContCreate(cont_check_cacheable, nullptr);
@@ -538,9 +558,6 @@ void
TSPluginInit(int argc, const char *argv[])
{
TSPluginRegistrationInfo info;
- static const struct option longopt[] = {{const_cast<char *>("log"), required_argument, nullptr, 'l'},
- {const_cast<char *>("config"), required_argument, nullptr, 'c'},
- {nullptr, no_argument, nullptr, '\0'}};
info.plugin_name = (char *)PLUGIN_NAME;
info.vendor_name = (char *)"Apache Software Foundation";
@@ -554,26 +571,18 @@ TSPluginInit(int argc, const char *argv[])
gConfig = new BgFetchConfig(cont);
- while (true) {
- int opt = getopt_long(argc, (char *const *)argv, "lc", longopt, nullptr);
-
- switch (opt) {
- case 'l':
- BgFetchState::getInstance().createLog(optarg);
- break;
- case 'c':
- TSDebug(PLUGIN_NAME, "config file '%s'", optarg);
- gConfig->readConfig(optarg);
- break;
- }
-
- if (opt == -1) {
- break;
+ if (gConfig->parseOptions(argc, argv)) {
+ // Create the global log file. Note that calling this multiple times currently has no
+ // effect, only one log file is ever created. The BgFetchState is a singleton.
+ if (!gConfig->logFile().empty()) {
+ BgFetchState::getInstance().createLog(gConfig->logFile());
}
+ TSDebug(PLUGIN_NAME, "Initialized");
+ TSHttpHookAdd(TS_HTTP_READ_RESPONSE_HDR_HOOK, cont);
+ } else {
+ // ToDo: Hmmm, no way to fail a global plugin here?
+ TSDebug(PLUGIN_NAME, "Failed to initialize as global plugin");
}
-
- TSDebug(PLUGIN_NAME, "Initialized");
- TSHttpHookAdd(TS_HTTP_READ_RESPONSE_HDR_HOOK, cont);
}
///////////////////////////////////////////////////////////////////////////
@@ -608,16 +617,40 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf */, int /
{
TSCont cont = TSContCreate(cont_handle_response, nullptr);
BgFetchConfig *config = new BgFetchConfig(cont);
-
- // Parse the optional rules, wihch becomes a linked list of BgFetchRule's.
- if (argc > 2) {
- TSDebug(PLUGIN_NAME, "config file %s", argv[2]);
- config->readConfig(argv[2]);
+ bool success = true;
+
+ // The first two arguments are the "from" and "to" URL string. We need to
+ // skip them, but we also require that there be an option to masquerade as
+ // argv[0], so we increment the argument indexes by 1 rather than by 2.
+ argc--;
+ argv++;
+
+ // This is for backwards compatibility, ugly! ToDo: Remove for ATS v9.0.0 IMO.
+ if (argc > 1 && *argv[1] != '-') {
+ TSDebug(PLUGIN_NAME, "config file %s", argv[1]);
+ if (!config->readConfig(argv[1])) {
+ success = false;
+ }
+ } else {
+ if (config->parseOptions(argc, const_cast<const char **>(argv))) {
+ // Create the global log file. Remember, the BgFetchState is a singleton.
+ if (config->logFile().size()) {
+ BgFetchState::getInstance().createLog(config->logFile());
+ }
+ } else {
+ success = false;
+ }
}
- *ih = (void *)config;
+ if (success) {
+ *ih = config;
- return TS_SUCCESS;
+ return TS_SUCCESS;
+ }
+
+ // Something went wrong with the configuration setup.
+ delete config;
+ return TS_ERROR;
}
void
@@ -625,6 +658,7 @@ TSRemapDeleteInstance(void *ih)
{
BgFetchConfig *config = static_cast<BgFetchConfig *>(ih);
+ TSContDestroy(config->getCont());
delete config;
}
@@ -644,11 +678,15 @@ TSRemapDoRemap(void *ih, TSHttpTxn txnp, TSRemapRequestInfo * /* rri */)
if (TS_SUCCESS == TSHttpTxnClientReqGet(txnp, &bufp, &req_hdrs)) {
TSMLoc field_loc = TSMimeHdrFieldFind(bufp, req_hdrs, TS_MIME_FIELD_RANGE, TS_MIME_LEN_RANGE);
+ if (!field_loc) { // Less common case, but also allow If-Range header to triger, but only if Range not present
+ field_loc = TSMimeHdrFieldFind(bufp, req_hdrs, TS_MIME_FIELD_IF_RANGE, TS_MIME_LEN_IF_RANGE);
+ }
+
if (field_loc) {
BgFetchConfig *config = static_cast<BgFetchConfig *>(ih);
TSHttpTxnHookAdd(txnp, TS_HTTP_READ_RESPONSE_HDR_HOOK, config->getCont());
- TSDebug(PLUGIN_NAME, "background fetch TSRemapDoRemap");
+ TSDebug(PLUGIN_NAME, "TSRemapDoRemap() added hook, request was Range / If-Range");
TSHandleMLocRelease(bufp, req_hdrs, field_loc);
}
TSHandleMLocRelease(bufp, TS_NULL_MLOC, req_hdrs);
diff --git a/plugins/background_fetch/configs.cc b/plugins/background_fetch/configs.cc
index 0322165..96fd251 100644
--- a/plugins/background_fetch/configs.cc
+++ b/plugins/background_fetch/configs.cc
@@ -22,10 +22,56 @@
limitations under the License.
*/
-#include "configs.h"
+#include <getopt.h>
#include <cstdio>
#include <memory.h>
+#include "configs.h"
+
+// Parse the command line options. This got a little wonky, since we decided to have different
+// syntax for remap vs global plugin initialization, and the global BG fetch state :-/. Clean up
+// later...
+bool
+BgFetchConfig::parseOptions(int argc, const char *argv[])
+{
+ static const struct option longopt[] = {{const_cast<char *>("log"), required_argument, nullptr, 'l'},
+ {const_cast<char *>("config"), required_argument, nullptr, 'c'},
+ {const_cast<char *>("allow-304"), no_argument, nullptr, 'a'},
+ {nullptr, no_argument, nullptr, '\0'}};
+
+ while (true) {
+ int opt = getopt_long(argc, (char *const *)argv, "lc", longopt, nullptr);
+
+ if (opt == -1) {
+ break;
+ }
+
+ switch (opt) {
+ case 'l':
+ TSDebug(PLUGIN_NAME, "option: log file specified: %s", optarg);
+ _log_file = optarg;
+ break;
+ case 'c':
+ TSDebug(PLUGIN_NAME, "option: config file '%s'", optarg);
+ if (!readConfig(optarg)) {
+ // Error messages are written in the parser
+ return false;
+ }
+ break;
+ case 'a':
+ TSDebug(PLUGIN_NAME, "option: --allow-304 set");
+ _allow_304 = true;
+ break;
+ default:
+ TSError("[%s] invalid plugin option: %c", PLUGIN_NAME, opt);
+ return false;
+ break;
+ }
+ }
+
+ return true;
+}
+
// Read a config file, populare the linked list (chain the BgFetchRule's)
bool
BgFetchConfig::readConfig(const char *config_file)
@@ -45,11 +91,12 @@ BgFetchConfig::readConfig(const char *config_file)
} else {
snprintf(file_path, sizeof(file_path), "%s/%s", TSConfigDirGet(), config_file);
}
- TSDebug(PLUGIN_NAME, "Chosen config file is at: %s", file_path);
+ TSDebug(PLUGIN_NAME, "chosen config file is at: %s", file_path);
file = TSfopen(file_path, "r");
if (nullptr == file) {
TSError("[%s] invalid config file: %s", PLUGIN_NAME, file_path);
+ TSDebug(PLUGIN_NAME, "invalid config file: %s", file_path);
return false;
}
diff --git a/plugins/background_fetch/configs.h b/plugins/background_fetch/configs.h
index 569ebde..ba6dd35 100644
--- a/plugins/background_fetch/configs.h
+++ b/plugins/background_fetch/configs.h
@@ -26,6 +26,7 @@
#include <cstdlib>
#include <atomic>
+#include <string>
#include "rules.h"
@@ -48,6 +49,8 @@ public:
}
}
+ bool parseOptions(int argc, const char *argv[]);
+
BgFetchRule *
getRules() const
{
@@ -60,12 +63,26 @@ public:
return _cont;
}
+ const std::string &
+ logFile() const
+ {
+ return _log_file;
+ }
+
+ bool
+ allow304() const
+ {
+ return _allow_304;
+ }
+
// This parses and populates the BgFetchRule linked list (_rules).
bool readConfig(const char *file_name);
bool bgFetchAllowed(TSHttpTxn txnp) const;
private:
- TSCont _cont;
- BgFetchRule *_rules{nullptr};
+ TSCont _cont = nullptr;
+ BgFetchRule *_rules = nullptr;
+ bool _allow_304 = false;
+ std::string _log_file;
};