[5647] CalloutHandle is now associated with the DHCP packet object.

When the packet goes out of scope the callout handle is destroyed.

src/lib/dhcp/Makefile.am

@@ -69,7 +69,11 @@ libkea_dhcp___la_CPPFLAGS = $(AM_CPPFLAGS)
 libkea_dhcp___la_LIBADD   = $(top_builddir)/src/lib/asiolink/libkea-asiolink.la
 libkea_dhcp___la_LIBADD  += $(top_builddir)/src/lib/dns/libkea-dns++.la
 libkea_dhcp___la_LIBADD  += $(top_builddir)/src/lib/cryptolink/libkea-cryptolink.la
+libkea_dhcp___la_LIBADD  += $(top_builddir)/src/lib/hooks/libkea-hooks.la
+libkea_dhcp___la_LIBADD  += $(top_builddir)/src/lib/log/libkea-log.la
+libkea_dhcp___la_LIBADD  += $(top_builddir)/src/lib/util/threads/libkea-threads.la
 libkea_dhcp___la_LIBADD  += $(top_builddir)/src/lib/util/libkea-util.la
+libkea_dhcp___la_LIBADD  += $(top_builddir)/src/lib/cc/libkea-cc.la
 libkea_dhcp___la_LIBADD  += $(top_builddir)/src/lib/exceptions/libkea-exceptions.la
 libkea_dhcp___la_LIBADD  += $(BOOST_LIBS)
 libkea_dhcp___la_LIBADD  += $(CRYPTO_LIBS)

src/lib/dhcp/pkt.h

@@ -12,6 +12,7 @@
 #include <dhcp/option.h>
 #include <dhcp/hwaddr.h>
 #include <dhcp/classify.h>
+#include <hooks/callout_handle_associate.h>
 
 #include <boost/date_time/posix_time/posix_time.hpp>
 #include <boost/shared_ptr.hpp>
@@ -86,7 +87,7 @@ private:
 ///
 /// @note This is abstract class. Please instantiate derived classes
 /// such as @c Pkt4 or @c Pkt6.
-class Pkt {
+class Pkt : public hooks::CalloutHandleAssociate {
 protected:
 
     /// @brief Constructor.

src/lib/dhcp/tests/Makefile.am

@@ -103,9 +103,11 @@ libdhcp___unittests_LDADD  = $(top_builddir)/src/lib/dhcp/libkea-dhcp++.la
 libdhcp___unittests_LDADD += $(top_builddir)/src/lib/asiolink/libkea-asiolink.la
 libdhcp___unittests_LDADD += $(top_builddir)/src/lib/dns/libkea-dns++.la
 libdhcp___unittests_LDADD += $(top_builddir)/src/lib/cryptolink/libkea-cryptolink.la
+libdhcp___unittests_LDADD += $(top_builddir)/src/lib/hooks/libkea-hooks.la
 libdhcp___unittests_LDADD += $(top_builddir)/src/lib/log/libkea-log.la
 libdhcp___unittests_LDADD += $(top_builddir)/src/lib/util/threads/libkea-threads.la
 libdhcp___unittests_LDADD += $(top_builddir)/src/lib/util/libkea-util.la
+libdhcp___unittests_LDADD += $(top_builddir)/src/lib/cc/libkea-cc.la
 libdhcp___unittests_LDADD += $(top_builddir)/src/lib/exceptions/libkea-exceptions.la
 libdhcp___unittests_LDADD += $(top_builddir)/src/lib/testutils/libkea-testutils.la
 libdhcp___unittests_LDADD += $(LOG4CPLUS_LIBS) $(CRYPTO_LIBS)

src/lib/dhcpsrv/callout_handle_store.h

@@ -19,86 +19,39 @@ namespace dhcp {
 ///
 /// When using the Hooks Framework, there is a need to associate an
 /// isc::hooks::CalloutHandle object with each request passing through the
-/// server.  For the DHCP servers, the association is provided by this function.
+/// server.  For the DHCP servers, the association was provided by this function.
 ///
-/// The DHCP servers process requests sequentially, but sometimes packets can
-/// be "parked" waiting for the completion of asynchronous operations scheduled
-/// by hook libraries. While the packets are parked, other packets can be
-/// processed. This poses a requirement for the DHCP server to keep
-/// multiple associations between the packets and their handles. At points where
-/// the CalloutHandle is required, the pointer to the current request (packet)
-/// is passed to this function. If the request is a new one, a pointer to the
-/// request stored, a new CalloutHandle is allocated (and stored) and a pointer
-/// to the latter object returned to the caller. If the request matches one of
-/// the stored requests, the CalloutHandle for this request is returned.
+/// After introduction of "packets parking" feature this function was extended
+/// to keep association of packets with the callout handles in a map.
+/// However, it was later found that "garbage collection" of the unused
+/// handles is very hard. Trying to garbage collect handles at each invocation
+/// was highly inefficient and caused server's performance degradation.
 ///
-/// In order to avoid the endless growth of the collection of stored associations
-/// this function performs housekeeping of the collection every time it is
-/// called with non-null packet pointer. All entries for which the packet
-/// pointer's reference count is less than 2 are removed. These are the packet
-/// objects which only this function holds reference counts to. Thus, they can
-/// be removed because they are not required by the server anymore. They have
-/// been either dropped or processed.
+/// The new approach is using on @c isc::hooks::CalloutHandleAssociate to
+/// associate objects with callout handles. This has a major benefit that
+/// callout handle instances are removed together with the packets associated
+/// with them.
 ///
-///
-/// A special case is a null pointer being passed to this function. This has
-/// the effect of clearing the pointers to stored handles and packets.
+/// This function uses this new approach and is kept for the compatibility with
+/// existing code.
 ///
 /// @tparam T Pkt4Ptr or Pkt6Ptr object.
 /// @param pktptr Pointer to the packet being processed.  This is typically a
-///        Pkt4Ptr or Pkt6Ptr object.  An empty pointer is passed to clear
-///        the stored pointers.
+///        Pkt4Ptr or Pkt6Ptr object.
 ///
-/// @return Shared pointer to a CalloutHandle.  This is the previously-stored
+/// @return Shared pointer to a CalloutHandle. This is the previously-stored
 ///         CalloutHandle if pktptr points to a packet that has been seen
-///         before or a new CalloutHandle if it points to a new one.  An empty
+///         before or a new CalloutHandle if it points to a new one. An empty
 ///         pointer is returned if pktptr is itself an empty pointer.
 template <typename T>
 isc::hooks::CalloutHandlePtr getCalloutHandle(const T& pktptr) {
 
-    isc::hooks::CalloutHandlePtr stored_handle;
-    static std::map<T, isc::hooks::CalloutHandlePtr> store;
-
     if (pktptr) {
+        return (pktptr->getCalloutHandle());
 
-        std::set<T> to_remove;
-
-        // Identify unused handles by checking whether the reference count is
-        // less then 2. This indicates that only our local store keeps the
-        // pointer to the packet.
-        for (auto handle_pair_it = store.begin(); handle_pair_it != store.end();
-             ++handle_pair_it) {
-
-            if (!handle_pair_it->first || handle_pair_it->first.use_count() < 2) {
-                to_remove.insert(handle_pair_it->first);
-            }
-        }
-
-        // Actually remove the unused handles.
-        for (const auto& pktptr : to_remove) {
-            store.erase(pktptr);
-        }
-
-        // Pointer given, have we seen it before?
-        auto stored_handle_it = store.find(pktptr);
-        if (stored_handle_it == store.end()) {
-
-            // Not seen before, so store the pointer passed to us and get a new
-            // CalloutHandle.
-            stored_handle = isc::hooks::HooksManager::createCalloutHandle();
-            store[pktptr] = stored_handle;
-
-        } else {
-            // Return existing pointer.
-            stored_handle = stored_handle_it->second;
-        }
-        
-    } else {
-        // Empty pointer passed, clear stored data
-        store.clear();
     }
 
-    return (stored_handle);
+    return (isc::hooks::CalloutHandlePtr());
 }
 
 } // namespace shcp

src/lib/dhcpsrv/tests/callout_handle_store_unittest.cc

@@ -33,24 +33,14 @@ TEST(CalloutHandleStoreTest, StoreRetrieve) {
     ASSERT_TRUE(pktptr_2);
 
     ASSERT_TRUE(pktptr_1 != pktptr_2);
-    EXPECT_EQ(1, pktptr_1.use_count());
-    EXPECT_EQ(1, pktptr_2.use_count());
 
     // Get the CalloutHandle for the first packet.
     CalloutHandlePtr chptr_1 = getCalloutHandle(pktptr_1);
     ASSERT_TRUE(chptr_1);
 
-    // Reference counts on both the callout handle and the packet should have
-    // been incremented because of the stored data.  The reference count on the
-    // other Pkt4 object should not have changed.
-    EXPECT_EQ(2, chptr_1.use_count());
-    EXPECT_EQ(2, pktptr_1.use_count());
-    EXPECT_EQ(1, pktptr_2.use_count());
-
     // Try getting another pointer for the same packet.  Use a different
     // pointer object to check that the function returns a handle based on the
-    // pointed-to data and not the pointer.  (Clear the temporary pointer after
-    // use to avoid complicating reference counts.)
+    // pointed-to data and not the pointer.
     Pkt4Ptr pktptr_temp = pktptr_1;
     CalloutHandlePtr chptr_2 = getCalloutHandle(pktptr_temp);
     pktptr_temp.reset();
@@ -58,53 +48,10 @@ TEST(CalloutHandleStoreTest, StoreRetrieve) {
     ASSERT_TRUE(chptr_2);
     EXPECT_TRUE(chptr_1 == chptr_2);
 
-    // Reference count is now 3 on the callout handle - two for pointers here,
-    // one for the static pointer in the function.  The count is 2 for the
-    // object pointed to by pktptr_1 - one for that pointer and one for the
-    // pointer in the function.
-    EXPECT_EQ(3, chptr_1.use_count());
-    EXPECT_EQ(3, chptr_2.use_count());
-    EXPECT_EQ(2, pktptr_1.use_count());
-    EXPECT_EQ(1, pktptr_2.use_count());
-
     // Now ask for a CalloutHandle for a different object.  This should return
     // a different CalloutHandle.
     chptr_2 = getCalloutHandle(pktptr_2);
     EXPECT_FALSE(chptr_1 == chptr_2);
-
-    // Check reference counts.  The getCalloutHandle function should be storing
-    // pointers to objects pointed to by chptr_1, pktptr_1, chptr_2 and pktptr_2.
-    EXPECT_EQ(2, chptr_1.use_count());
-    EXPECT_EQ(2, pktptr_1.use_count());
-    EXPECT_EQ(2, chptr_2.use_count());
-    EXPECT_EQ(2, pktptr_2.use_count());
-
-    // Now clear the pktptr_1 simulating the situation when the DHCP packet
-    // has been fully processed.
-    pktptr_1.reset();
-
-    // Retrieve chptr_2 again to trigger clearing unused pointer chptr_1.
-    chptr_2 = getCalloutHandle(pktptr_2);
-    ASSERT_TRUE(chptr_2);
-
-    // This should merely affect chptr_1 and pktptr_1, but the pointers for the
-    // second packet should remain unchanged.
-    EXPECT_EQ(1, chptr_1.use_count());
-    EXPECT_EQ(2, chptr_2.use_count());
-    EXPECT_EQ(2, pktptr_2.use_count());
-
-    // Now try clearing the stored pointers.
-    Pkt4Ptr pktptr_empty;
-    ASSERT_FALSE(pktptr_empty);
-
-    CalloutHandlePtr chptr_empty = getCalloutHandle(pktptr_empty);
-    EXPECT_FALSE(chptr_empty);
-
-    // Reference counts should be back to 1 for the CalloutHandles and the
-    // Packet pointers.
-    EXPECT_EQ(1, chptr_1.use_count());
-    EXPECT_EQ(1, chptr_2.use_count());
-    EXPECT_EQ(1, pktptr_2.use_count());
 }
 
 // The followings is a trivial test to check that if the template function

src/lib/hooks/Makefile.am

@@ -32,6 +32,7 @@ CLEANFILES = *.gcno *.gcda hooks_messages.h hooks_messages.cc s-messages
 lib_LTLIBRARIES = libkea-hooks.la
 libkea_hooks_la_SOURCES  =
 libkea_hooks_la_SOURCES += callout_handle.cc callout_handle.h
+libkea_hooks_la_SOURCES += callout_handle_associate.cc callout_handle_associate.h
 libkea_hooks_la_SOURCES += callout_manager.cc callout_manager.h
 libkea_hooks_la_SOURCES += hooks.h
 libkea_hooks_la_SOURCES += hooks_log.cc hooks_log.h

src/lib/hooks/callout_handle_associate.cc

+// Copyright (C) 2018 Internet Systems Consortium, Inc. ("ISC")
+//
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+#include <hooks/callout_handle_associate.h>
+#include <hooks/hooks_manager.h>
+
+namespace isc {
+namespace hooks {
+
+CalloutHandleAssociate::CalloutHandleAssociate()
+    : callout_handle_() {
+}
+
+CalloutHandlePtr
+CalloutHandleAssociate::getCalloutHandle() {
+    if (!callout_handle_) {
+        callout_handle_ = HooksManager::createCalloutHandle();
+    }
+
+    return (callout_handle_);
+}
+
+} // end of namespace isc::hooks
+} // end of namespace isc

src/lib/hooks/callout_handle_associate.h

+// Copyright (C) 2018 Internet Systems Consortium, Inc. ("ISC")
+//
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+#ifndef CALLOUT_HANDLE_ASSOCIATE_H
+#define CALLOUT_HANDLE_ASSOCIATE_H
+
+#include <hooks/callout_handle.h>
+
+namespace isc {
+namespace hooks {
+
+/// @brief Base class for classes which need to be associated with
+/// a @c CalloutHandle object.
+///
+/// The @c CalloutHandle is an object used to pass various parameters
+/// between Kea and the callouts. The Kea servers usually invoke
+/// multiple different callouts for a single packet such as DHCP
+/// packet, control command etc. Therefore, it is required to
+/// associate this packet with an instance of the callout handle, so
+/// this instance can be used for all callouts invoked for this
+/// packet.
+///
+/// Previously this association was made by the @c CalloutHandleStore
+/// class. However, with the introduction of parallel processing
+/// of packets (DHCP packets parking) it became awkward to use.
+/// Attempts to extend this class to hold a map of associations
+/// failed because of no easy way to garbage collect unused handles.
+///
+/// The easiest way to deal with this is to provide ownership of the
+/// @c CalloutHandle to the object with which it is associated. The
+/// class of this object needs to derive from this class. When the
+/// object (e.g. DHCP packet) goes out of scope and is destroyed
+/// this instance is destroyed as well.
+class CalloutHandleAssociate {
+public:
+
+    /// @brief Constructor.
+    CalloutHandleAssociate();
+
+    /// @brief Returns callout handle.
+    ///
+    /// The callout handle is created if it doesn't exist. Subsequent
+    /// calls to this method always return the same handle.
+    ///
+    /// @return Pointer to the callout handle.
+    CalloutHandlePtr getCalloutHandle();
+
+protected:
+
+    /// @brief Callout handle stored.
+    CalloutHandlePtr callout_handle_;
+
+
+};
+
+} // end of isc::hooks
+} // end of isc
+
+#endif

src/lib/hooks/tests/Makefile.am

@@ -111,6 +111,7 @@ libacl_la_LDFLAGS  = -avoid-version -export-dynamic -module -rpath /nowhere
 TESTS += run_unittests
 run_unittests_SOURCES  = run_unittests.cc
 run_unittests_SOURCES += callout_handle_unittest.cc
+run_unittests_SOURCES += callout_handle_associate_unittest.cc
 run_unittests_SOURCES += callout_manager_unittest.cc
 run_unittests_SOURCES += common_test_class.h
 run_unittests_SOURCES += handles_unittest.cc

src/lib/hooks/tests/callout_handle_associate_unittest.cc

+// Copyright (C) 2018 Internet Systems Consortium, Inc. ("ISC")
+//
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+#include <config.h>
+
+#include <hooks/callout_handle.h>
+#include <hooks/callout_handle_associate.h>
+#include <gtest/gtest.h>
+
+using namespace isc::hooks;
+
+namespace {
+
+// This test verifies that the callout handle can be created and
+// retrieved from the CalloutHandleAssociate.
+TEST(CalloutHandleAssociate, getCalloutHandle) {
+    CalloutHandleAssociate associate;
+    // The handle should be initialized and returned.
+    CalloutHandlePtr callout_handle = associate.getCalloutHandle();
+    ASSERT_TRUE(callout_handle);
+
+    // When calling the second time, the same handle should be returned.
+    CalloutHandlePtr callout_handle2 = associate.getCalloutHandle();
+    EXPECT_TRUE(callout_handle == callout_handle2);
+
+    // A different associate should produce a different handle.
+    CalloutHandleAssociate associate2;
+    callout_handle2 = associate2.getCalloutHandle();
+    EXPECT_FALSE(callout_handle == callout_handle2);
+}
+
+} // end of anonymous namespace