Rearrange config reload notifications

Renamed `TSRemapConfigReload` to `TSRemapPreConfigReload` to notify
all already loaded plugins that a configuration (re)load is about to
start.

Added new `TSRemapPostConfigReload(TSReturnCode)` to notify the plugins
that the configuration (re)load is done and provide status of its
success or failure.

build/plugins.mk

@@ -22,7 +22,7 @@ TS_PLUGIN_LD_FLAGS = \
   -module \
   -shared \
   -avoid-version \
-  -export-symbols-regex '^(TSRemapInit|TSRemapDone|TSRemapDoRemap|TSRemapNewInstance|TSRemapDeleteInstance|TSRemapOSResponse|TSPluginInit)$$'
+  -export-symbols-regex '^(TSRemapInit|TSRemapDone|TSRemapDoRemap|TSRemapNewInstance|TSRemapDeleteInstance|TSRemapOSResponse|TSPluginInit|TSRemapPreConfigReload|TSRemapPostConfigReload)$$'
 
 TS_PLUGIN_CPPFLAGS = \
   -I$(abs_top_builddir)/proxy/api \

doc/developer-guide/api/functions/TSRemap.en.rst

@@ -30,7 +30,8 @@ Synopsis
 `#include <ts/remap.h>`
 
 .. function:: TSReturnCode TSRemapInit(TSRemapInterface * api_info, char * errbuff, int errbuff_size)
-.. function:: void TSRemapConfigReload(void)
+.. function:: void TSRemapPreConfigReload(void)
+.. function:: void TSRemapPostConfigReload(TSReturnCode reloadStatus)
 .. function:: void TSRemapDone(void)
 .. function:: TSRemapStatus TSRemapDoRemap(void * ih, TSHttpTxn rh, TSRemapRequestInfo * rri)
 .. function:: TSReturnCode TSRemapNewInstance(int argc, char * argv[], void ** ih, char * errbuff, int errbuff_size)
@@ -65,9 +66,14 @@ any data or continuations associated with that instance.
 entry point. In this function, the remap plugin may examine and modify
 the HTTP transaction.
 
-:func:`TSRemapConfigReload` is called once for every remap plugin immediately after a new
-configuration is successfully loaded and immediately before the new remap configuration becomes
-active. This is an optional entry point, which takes no arguments and has no return value.
+:func:`TSRemapPreConfigReload` is called *before* the parsing of a new remap configuration starts
+to notify plugins of the coming configuration reload. It is called on all already loaded plugins,
+invoked by current and all previous still used configurations. This is an optional entry point.
+
+:func:`TSRemapPostConfigReload` is called to indicate the end of the the new remap configuration
+load. It is called on the newly and previously loaded plugins, invoked by the new, current and
+previous still used configurations. It also indicates if the configuration reload was successful
+by passing :macro:`TS_SUCCESS` or :macro:`TS_ERROR`. This is an optional entry point.
 
 Generally speaking, calls to these functions are mutually exclusive. The exception
 is for functions which take an HTTP transaction as a parameter. Calls to these

doc/developer-guide/plugins/remap-plugins.en.rst

@@ -80,12 +80,46 @@ multiple times for the rule, once for each invocation. Only the value store in :
 available when the rule is actually matched. In particular the plugin arguments will not be
 available.
 
-Calls to :func:`TSRemapNewInstance` are serialized.
+Calls to :func:`TSRemapNewInstance` are serialized. All calls to :func:`TSRemapNewInstance`
+for a given configuration will be called and completed before any calls to :func:`TSRemapDoRemap`.
 
 If there is an error then the callback should return :macro:`TS_ERROR` and fill in the
 :arg:`errbuff` with a C-string describing the error. Otherwise the function must return
 :macro:`TS_SUCCESS`.
 
+
+Configuration reload notifications
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+:func:`TSRemapPreConfigReload` is called *before* the parsing of a new remap configuration starts
+to notify plugins of the coming configuration reload. It is called on all already loaded plugins,
+invoked by current and all previous still used configurations. This is an optional entry point.
+
+:func:`TSRemapPostConfigReload` is called to indicate the end of the the new remap configuration
+load. It is called on the newly and previously loaded plugins, invoked by the new, current and
+previous still used configurations. It also indicates if the configuration reload was successful
+by passing :macro:`TS_SUCCESS` or :macro:`TS_ERROR`. This is an optional entry point.
+
+These calls are called per *plugin*, not per invocation of the plugin in :file:`remap.config`
+and only will be called if the plugin was called at least once with :func:`TSRemapNewInstance`
+for any configuration and at least one configuration using it is still loaded.
+
+:func:`TSRemapPreConfigReload` will be called serially for all loaded plugins
+before any call to :func:`TSRemapNewInstance` during parsing of the new configuration.
+
+:func:`TSRemapPostConfigReload` will be called serially for all plugins after
+all calls to :func:`TSRemapNewInstance` during parsing of the new configuration.
+
+The intention of these callbacks can be demonstrated with the following use-case.
+A plugin could use :func:`TSRemapPreConfigReload` as a signal to drop (or allocate) temporary
+per plugin data structures. These structures can be created (or updated) as needed
+when a plugin invocation instance is loaded (:func:`TSRemapNewInstance` is called).
+Then it could be used in subsequent invocation instances loading. After the configuration
+reload is done :func:`TSRemapPostConfigReload` could be used to confirm
+the data structures update if :macro:`TS_SUCCESS` is passed or recover / clean-up
+after a failed reload attempt if :macro:`TS_ERROR` is passed.
+
+
 Runtime
 =======
 
@@ -104,8 +138,13 @@ Calls to :func:`TSRemapDoRemap` are not serialized, they can be concurrent, even
 invocation instance. However, the callbacks for a single rule for a single transaction are
 serialized in the order the plugins are invoked in the rule.
 
-All calls to :func:`TSRemapNewInstance` for a given configuration will be called and completed
-before any calls to :func:`TSRemapDoRemap`.
+No calls to :func:`TSRemapDoRemap` will occur before :func:`TSRemapPostConfigReload` for
+all plugin instances invoked by the new configuration.
+
+The old configurations, if any, are still active during the calls to :func:`TSRemapPreConfigReload`
+and :func:`TSRemapPreConfigReload` and therefore calls to :func:`TSRemapDoRemap` may occur
+concurrently with those functions.
+
 
 Cleanup
 =======
@@ -114,35 +153,9 @@ When a new :file:`remap.config` is loaded successfully, the prior configuration
 each call to :func:`TSRemapNewInstance` a corresponding call to :func:`TSRemapDeleteInstance` is
 called. The only argument is the invocation instance handle, originally provided by the plugin to
 :func:`TSRemapNewInstance`. This is expected to suffice for the plugin to clean up any rule specific
-data.
-
-As part of the old configuration cleanup :func:`TSRemapConfigReload` is called on the plugins in the
-old configuration before any calls to :func:`TSRemapDeleteInstance`. This is an optional entry
-point.
-
-.. note::
-
-   This is called per *plugin*, not per invocation of the plugin in :file:`remap.config`, and only
-   called if the plugin was called at least once with :func:`TSRemapNewInstance` for that
-   configuration.
-
-.. note::
-
-   There is no explicit indication or connection between the call to :func:`TSRemapConfigReload` and
-   the "old" (existing) configuration. It is guaranteeed that :func:`TSRemapConfigReload` will be
-   called on all the plugins before any :func:`TSRemapDeleteInstance` and these calls will be
-   serial. Similarly, :func:`TSRemapConfigReload` will be called serially after all calls to
-   :func:`TSRemapNewInstance` for a given configuration.
-
-.. note::
-
-   The old configuration, if any, is still active during the call to :func:`TSRemapConfigReload` and
-   therefore calls to :func:`TSRemapDoRemap` may occur concurrently with that function.
-
-The intention of :func:`TSRemapConfigReload` is to provide for temporary data structures used only
-during configuration loading. These can be created as needed when an invocation instance is loaded
-and used in subsequent invocation instance loading, then cleaned up in :func:`TSRemapConfigReload`.
+data. Calls to :func:`TSRemapDeleteInstance` will be serial for all plugin invocations in a
+remap configuration.
 
 If no rule uses a plugin, it may be unloaded. In that case :func:`TSRemapDone` is called. This is
-an optional entry point, a plugin is not required to provide it. It is called once per plugin, not
-per plugin invocation.
+an optional entry point, a plugin is not required to provide it. It corresponds to :func:`TSRemapInit`.
+It is called once per plugin, not per plugin invocation.

include/ts/remap.h

@@ -88,13 +88,27 @@ typedef enum {
 */
 tsapi TSReturnCode TSRemapInit(TSRemapInterface *api_info, char *errbuf, int errbuf_size);
 
-/* This gets called every time remap.config is reloaded. This is complementary
-   to TSRemapInit() which gets called when the plugin is first loaded. You can
-   not fail, or cause reload to stop here, it's merely a notification.
+/* This gets called every time before remap.config is reloaded. This is complementary
+   to TSRemapInit() which gets called when the plugin is first loaded.
+   It is guaranteed to be called before TSRemapInit() and TSRemapNewInstance().
+   It cannot fail, or cause reload to stop here, it's merely a notification.
    Optional function.
+   Params: none
    Return: none
 */
-tsapi void TSRemapConfigReload(void);
+tsapi void TSRemapPreConfigReload(void);
+
+/* This gets called every time afterremap.config is reloaded. This is complementary
+   to TSRemapInit() which gets called when the plugin is first loaded.
+   It is guaranteed to be called after TSRemapInit() and TSRemapNewInstance().
+   It cannot fail, or cause reload to stop here, it's merely a notification that
+   the (re)load is done and provide a status of its success or failure..
+   Optional function.
+   Params: reloadStatus - TS_SUCCESS - (re)load was successful,
+                          TS_ERROR - (re)load failed.
+   Return: none
+*/
+tsapi void TSRemapPostConfigReload(TSReturnCode reloadStatus);
 
 /* Remap new request
    Mandatory interface function.

proxy/ReverseProxy.cc

@@ -151,7 +151,7 @@ reloadUrlRewrite()
     ink_assert(oldTable != nullptr);
 
     // Release the old one
-    oldTable->pluginFactory.indicateReload();
+    oldTable->pluginFactory.deactivate();
     oldTable->release();
 
     Debug("url_rewrite", "%s", msg);

proxy/http/remap/Makefile.am

@@ -103,7 +103,7 @@ DSO_LDFLAGS = \
 	-module \
 	-shared \
 	-avoid-version \
-	-export-symbols-regex '^(TSRemapInit|TSRemapDone|TSRemapDoRemap|TSRemapNewInstance|TSRemapDeleteInstance|TSRemapOSResponse|TSRemapConfigReload|TSPluginInit|pluginDsoVersionTest|getPluginDebugObjectTest)$$'
+	-export-symbols-regex '^(TSRemapInit|TSRemapDone|TSRemapDoRemap|TSRemapNewInstance|TSRemapDeleteInstance|TSRemapOSResponse|TSRemapPreConfigReload|TSRemapPostConfigReload|TSPluginInit|pluginDsoVersionTest|getPluginDebugObjectTest)$$'
 
 # Build plugins for unit testing the plugin (re)load.
 pkglib_LTLIBRARIES = \

proxy/http/remap/PluginDso.h

@@ -33,6 +33,7 @@
 #include <vector>
 #include <ctime>
 
+#include "ts/apidefs.h"
 #include "tscore/ts_file.h"
 namespace fs = ts::file;
 
@@ -73,10 +74,11 @@ class PluginDso : public PluginThreadContext
   using Linkage    = ts::IntrusiveLinkage<self_type>;
   using PluginList = ts::IntrusiveDList<PluginDso::Linkage>;
 
-  /* Methods to be called when processing a list of plugins, to overloaded by the remap or the global plugins correspondingly */
-  virtual void indicateReload()         = 0;
-  virtual bool init(std::string &error) = 0;
-  virtual void done()                   = 0;
+  /* Methods to be called when processing a list of plugins, to be overloaded by the remap or the global plugins correspondingly */
+  virtual void indicatePreReload()                           = 0;
+  virtual void indicatePostReload(TSReturnCode reloadStatus) = 0;
+  virtual bool init(std::string &error)                      = 0;
+  virtual void done()                                        = 0;
 
   void acquire();
   void release();

proxy/http/remap/PluginFactory.cc

@@ -35,28 +35,34 @@
 RemapPluginInst::RemapPluginInst(RemapPluginInfo &plugin) : _plugin(plugin)
 {
   _plugin.acquire();
-  _plugin.incInstanceCount();
 }
 
 RemapPluginInst::~RemapPluginInst()
 {
-  _plugin.decInstanceCount();
   _plugin.release();
 }
 
-bool
-RemapPluginInst::init(int argc, char **argv, std::string &error)
+RemapPluginInst *
+RemapPluginInst::init(RemapPluginInfo *plugin, int argc, char **argv, std::string &error)
 {
-  bool result = false;
-  result      = _plugin.initInstance(argc, argv, &_instance, error);
-
-  return result;
+  RemapPluginInst *inst = new RemapPluginInst(*plugin);
+  if (plugin->initInstance(argc, argv, &(inst->_instance), error)) {
+    plugin->incInstanceCount();
+    return inst;
+  }
+  delete inst;
+  return nullptr;
 }
 
 void
 RemapPluginInst::done()
 {
+  _plugin.decInstanceCount();
   _plugin.doneInstance(_instance);
+
+  if (0 == _plugin.instanceCount()) {
+    _plugin.done();
+  }
 }
 
 TSRemapStatus
@@ -163,9 +169,10 @@ PluginFactory::getRemapPlugin(const fs::path &configPath, int argc, char **argv,
         _list.append(plugin);
 
         if (plugin->init(error)) {
-          inst = new RemapPluginInst(*plugin);
-          inst->init(argc, argv, error);
-          _instList.append(inst);
+          inst = RemapPluginInst::init(plugin, argc, argv, error);
+          if (nullptr != inst) {
+            _instList.append(inst);
+          }
         }
 
         if (_preventiveCleaning) {
@@ -177,9 +184,10 @@ PluginFactory::getRemapPlugin(const fs::path &configPath, int argc, char **argv,
     }
   } else {
     Debug(_tag, "plugin '%s' has already been loaded", configPath.c_str());
-    inst = new RemapPluginInst(*plugin);
-    inst->init(argc, argv, error);
-    _instList.append(inst);
+    inst = RemapPluginInst::init(plugin, argc, argv, error);
+    if (nullptr != inst) {
+      _instList.append(inst);
+    }
   }
 
   return inst;
@@ -236,25 +244,44 @@ PluginFactory::findByEffectivePath(const fs::path &path)
 }
 
 /**
- * @brief Tell all plugins (that so wish) that remap.config is being reloaded
+ * @brief Tell all plugins instantiated by this factory that the configuration
+ * they are using is no longer the active one.
  *
  * This method would be useful only in case configs are reloaded independently from
  * factory/plugins instantiation and initialization.
  */
 void
-PluginFactory::indicateReload()
+PluginFactory::deactivate()
 {
-  Debug(_tag, "indicated config reload to factory '%s'", getUuid());
+  Debug(_tag, "deactivate configuration used by factory '%s'", getUuid());
 
   _instList.apply([](RemapPluginInst &pluginInst) -> void { pluginInst.done(); });
+}
 
-  _list.apply([](PluginDso &plugin) -> void {
-    if (1 == plugin.instanceCount()) {
-      plugin.done();
-    } else {
-      plugin.indicateReload();
-    }
-  });
+/**
+ * @brief Tell all plugins (that so wish) that remap.config is going to be reloaded
+ */
+void
+PluginFactory::indicatePreReload()
+{
+  Debug(_tag, "indicated config is going to be reloaded by factory '%s' to %zu plugin%s", getUuid(), _list.count(),
+        _list.count() != 1 ? "s" : "");
+
+  _list.apply([](PluginDso &plugin) -> void { plugin.indicatePreReload(); });
+}
+
+/**
+ * @brief Tell all plugins (that so wish) that remap.config is done reloading
+ */
+void
+PluginFactory::indicatePostReload(TSReturnCode reloadStatus)
+{
+  Debug(_tag, "indicated config is done reloading by factory '%s' to %zu plugin%s", getUuid(), _list.count(),
+        _list.count() != 1 ? "s" : "");
+
+  for (auto it = _list.begin(); _list.end() != it; it++) {
+    it->indicatePostReload(reloadStatus);
+  }
 }
 
 void

proxy/http/remap/PluginFactory.h

@@ -47,7 +47,7 @@ class RemapPluginInst
   ~RemapPluginInst();
 
   /* Used by the PluginFactory */
-  bool init(int argc, char **argv, std::string &error);
+  static RemapPluginInst *init(RemapPluginInfo *plugin, int argc, char **argv, std::string &error);
   void done();
 
   /* Used by the traffic server core while processing requests */
@@ -100,7 +100,9 @@ class PluginFactory
   virtual const char *getUuid();
   void clean(std::string &error);
 
-  void indicateReload();
+  void deactivate();
+  void indicatePreReload();
+  void indicatePostReload(TSReturnCode reloadStatus);
 
 protected:
   PluginDso *findByEffectivePath(const fs::path &path);

proxy/http/remap/RemapConfig.cc

@@ -1320,9 +1320,16 @@ remap_parse_config(const char *path, UrlRewrite *rewrite)
 {
   BUILD_TABLE_INFO bti;
 
-  // If this happens to be a config reload, the list of loaded remap plugins is non-empty, and we
-  // can signal all these plugins that a reload has begun.
-  rewrite->pluginFactory.indicateReload();
+  /* If this happens to be a config reload, the list of loaded remap plugins is non-empty, and we
+   * can signal all these plugins that a reload has begun. */
+  rewrite->pluginFactory.indicatePreReload();
+
   bti.rewrite = rewrite;
-  return remap_parse_config_bti(path, &bti);
+  bool status = remap_parse_config_bti(path, &bti);
+
+  /* Now after we parsed the configuration and (re)loaded plugins and plugin instances
+   * accordingly notify all plugins that we are done */
+  rewrite->pluginFactory.indicatePostReload(status ? TS_SUCCESS : TS_ERROR);
+
+  return status;
 }