From faf336d468a04979747c32895ac902d7e55c5483 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Tue, 12 Jun 2018 16:33:09 +0200 Subject: [PATCH] [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 | 4 + src/lib/dhcp/pkt.h | 3 +- src/lib/dhcp/tests/Makefile.am | 2 + src/lib/dhcpsrv/callout_handle_store.h | 81 ++++--------------- .../tests/callout_handle_store_unittest.cc | 55 +------------ src/lib/hooks/Makefile.am | 1 + src/lib/hooks/callout_handle_associate.cc | 27 +++++++ src/lib/hooks/callout_handle_associate.h | 62 ++++++++++++++ src/lib/hooks/tests/Makefile.am | 1 + .../callout_handle_associate_unittest.cc | 35 ++++++++ 10 files changed, 152 insertions(+), 119 deletions(-) create mode 100644 src/lib/hooks/callout_handle_associate.cc create mode 100644 src/lib/hooks/callout_handle_associate.h create mode 100644 src/lib/hooks/tests/callout_handle_associate_unittest.cc diff --git a/src/lib/dhcp/Makefile.am b/src/lib/dhcp/Makefile.am index e27d06ab44..6ce29d7332 100644 --- a/src/lib/dhcp/Makefile.am +++ b/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) diff --git a/src/lib/dhcp/pkt.h b/src/lib/dhcp/pkt.h index f2efc14de8..53ab96a6c7 100644 --- a/src/lib/dhcp/pkt.h +++ b/src/lib/dhcp/pkt.h @@ -12,6 +12,7 @@ #include #include #include +#include #include #include @@ -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. diff --git a/src/lib/dhcp/tests/Makefile.am b/src/lib/dhcp/tests/Makefile.am index 8993829733..40734f4f72 100644 --- a/src/lib/dhcp/tests/Makefile.am +++ b/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) diff --git a/src/lib/dhcpsrv/callout_handle_store.h b/src/lib/dhcpsrv/callout_handle_store.h index 43d8d56cb7..ccef4355f2 100644 --- a/src/lib/dhcpsrv/callout_handle_store.h +++ b/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 isc::hooks::CalloutHandlePtr getCalloutHandle(const T& pktptr) { - isc::hooks::CalloutHandlePtr stored_handle; - static std::map store; - if (pktptr) { + return (pktptr->getCalloutHandle()); - std::set 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 diff --git a/src/lib/dhcpsrv/tests/callout_handle_store_unittest.cc b/src/lib/dhcpsrv/tests/callout_handle_store_unittest.cc index 9498b66999..380ed58f9f 100644 --- a/src/lib/dhcpsrv/tests/callout_handle_store_unittest.cc +++ b/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 diff --git a/src/lib/hooks/Makefile.am b/src/lib/hooks/Makefile.am index 3de415f0b2..136e9ec129 100644 --- a/src/lib/hooks/Makefile.am +++ b/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 diff --git a/src/lib/hooks/callout_handle_associate.cc b/src/lib/hooks/callout_handle_associate.cc new file mode 100644 index 0000000000..d6e408e4ab --- /dev/null +++ b/src/lib/hooks/callout_handle_associate.cc @@ -0,0 +1,27 @@ +// 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 +#include + +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 diff --git a/src/lib/hooks/callout_handle_associate.h b/src/lib/hooks/callout_handle_associate.h new file mode 100644 index 0000000000..7445bbbb3b --- /dev/null +++ b/src/lib/hooks/callout_handle_associate.h @@ -0,0 +1,62 @@ +// 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 + +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 diff --git a/src/lib/hooks/tests/Makefile.am b/src/lib/hooks/tests/Makefile.am index 417cacb146..74951c108c 100644 --- a/src/lib/hooks/tests/Makefile.am +++ b/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 diff --git a/src/lib/hooks/tests/callout_handle_associate_unittest.cc b/src/lib/hooks/tests/callout_handle_associate_unittest.cc new file mode 100644 index 0000000000..cf09388680 --- /dev/null +++ b/src/lib/hooks/tests/callout_handle_associate_unittest.cc @@ -0,0 +1,35 @@ +// 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 + +#include +#include +#include + +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 -- 2.47.2