Commit eea88d5c authored by Marcin Siodelski's avatar Marcin Siodelski

[master] Merge branch 'trac5647'

parents 092b634f c186f7a8
......@@ -37,7 +37,11 @@ perfdhcp_LDADD += $(top_builddir)/src/lib/dhcp/libkea-dhcp++.la
perfdhcp_LDADD += $(top_builddir)/src/lib/asiolink/libkea-asiolink.la
perfdhcp_LDADD += $(top_builddir)/src/lib/dns/libkea-dns++.la
perfdhcp_LDADD += $(top_builddir)/src/lib/cryptolink/libkea-cryptolink.la
perfdhcp_LDADD += $(top_builddir)/src/lib/hooks/libkea-hooks.la
perfdhcp_LDADD += $(top_builddir)/src/lib/log/libkea-log.la
perfdhcp_LDADD += $(top_builddir)/src/lib/util/threads/libkea-threads.la
perfdhcp_LDADD += $(top_builddir)/src/lib/util/libkea-util.la
perfdhcp_LDADD += $(top_builddir)/src/lib/cc/libkea-cc.la
perfdhcp_LDADD += $(top_builddir)/src/lib/exceptions/libkea-exceptions.la
perfdhcp_LDADD += $(CRYPTO_LIBS)
perfdhcp_LDADD += $(BOOST_LIBS)
......
......@@ -41,8 +41,12 @@ run_unittests_LDADD += $(top_builddir)/src/lib/dhcp/libkea-dhcp++.la
run_unittests_LDADD += $(top_builddir)/src/lib/asiolink/libkea-asiolink.la
run_unittests_LDADD += $(top_builddir)/src/lib/dns/libkea-dns++.la
run_unittests_LDADD += $(top_builddir)/src/lib/cryptolink/libkea-cryptolink.la
run_unittests_LDADD += $(top_builddir)/src/lib/hooks/libkea-hooks.la
run_unittests_LDADD += $(top_builddir)/src/lib/log/libkea-log.la
run_unittests_LDADD += $(top_builddir)/src/lib/util/threads/libkea-threads.la
run_unittests_LDADD += $(top_builddir)/src/lib/util/unittests/libutil_unittests.la
run_unittests_LDADD += $(top_builddir)/src/lib/util/libkea-util.la
run_unittests_LDADD += $(top_builddir)/src/lib/cc/libkea-cc.la
run_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libkea-exceptions.la
run_unittests_LDADD += $(CRYPTO_LIBS) $(BOOST_LIBS) $(GTEST_LDADD)
endif
......
......@@ -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)
......
......@@ -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.
......
......@@ -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)
......
......@@ -43,6 +43,7 @@ libkea_dhcp_ddns_la_LIBADD += $(top_builddir)/src/lib/asiolink/libkea-asiolink.l
libkea_dhcp_ddns_la_LIBADD += $(top_builddir)/src/lib/cc/libkea-cc.la
libkea_dhcp_ddns_la_LIBADD += $(top_builddir)/src/lib/dns/libkea-dns++.la
libkea_dhcp_ddns_la_LIBADD += $(top_builddir)/src/lib/cryptolink/libkea-cryptolink.la
libkea_dhcp_ddns_la_LIBADD += $(top_builddir)/src/lib/hooks/libkea-hooks.la
libkea_dhcp_ddns_la_LIBADD += $(top_builddir)/src/lib/log/libkea-log.la
libkea_dhcp_ddns_la_LIBADD += $(top_builddir)/src/lib/util/threads/libkea-threads.la
libkea_dhcp_ddns_la_LIBADD += $(top_builddir)/src/lib/util/libkea-util.la
......
......@@ -38,6 +38,7 @@ libdhcp_ddns_unittests_LDADD += $(top_builddir)/src/lib/asiolink/libkea-asiolink
libdhcp_ddns_unittests_LDADD += $(top_builddir)/src/lib/cc/libkea-cc.la
libdhcp_ddns_unittests_LDADD += $(top_builddir)/src/lib/dns/libkea-dns++.la
libdhcp_ddns_unittests_LDADD += $(top_builddir)/src/lib/cryptolink/libkea-cryptolink.la
libdhcp_ddns_unittests_LDADD += $(top_builddir)/src/lib/hooks/libkea-hooks.la
libdhcp_ddns_unittests_LDADD += $(top_builddir)/src/lib/log/libkea-log.la
libdhcp_ddns_unittests_LDADD += $(top_builddir)/src/lib/util/threads/libkea-threads.la
libdhcp_ddns_unittests_LDADD += $(top_builddir)/src/lib/util/libkea-util.la
......
......@@ -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
......
......@@ -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
......
......@@ -30,6 +30,7 @@ libkea_eval_la_LIBADD = $(top_builddir)/src/lib/dhcp/libkea-dhcp++.la
libkea_eval_la_LIBADD += $(top_builddir)/src/lib/asiolink/libkea-asiolink.la
libkea_eval_la_LIBADD += $(top_builddir)/src/lib/dns/libkea-dns++.la
libkea_eval_la_LIBADD += $(top_builddir)/src/lib/cryptolink/libkea-cryptolink.la
libkea_eval_la_LIBADD += $(top_builddir)/src/lib/hooks/libkea-hooks.la
libkea_eval_la_LIBADD += $(top_builddir)/src/lib/log/libkea-log.la
libkea_eval_la_LIBADD += $(top_builddir)/src/lib/util/threads/libkea-threads.la
libkea_eval_la_LIBADD += $(top_builddir)/src/lib/util/libkea-util.la
......
......@@ -33,6 +33,7 @@ libeval_unittests_LDADD += $(top_builddir)/src/lib/dhcp/libkea-dhcp++.la
libeval_unittests_LDADD += $(top_builddir)/src/lib/asiolink/libkea-asiolink.la
libeval_unittests_LDADD += $(top_builddir)/src/lib/dns/libkea-dns++.la
libeval_unittests_LDADD += $(top_builddir)/src/lib/cryptolink/libkea-cryptolink.la
libeval_unittests_LDADD += $(top_builddir)/src/lib/hooks/libkea-hooks.la
libeval_unittests_LDADD += $(top_builddir)/src/lib/log/libkea-log.la
libeval_unittests_LDADD += $(top_builddir)/src/lib/util/threads/libkea-threads.la
libeval_unittests_LDADD += $(top_builddir)/src/lib/util/libkea-util.la
......
......@@ -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
......
// 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
// 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
......@@ -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
......
// 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
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment