From: Marcin Siodelski Date: Mon, 7 May 2018 11:18:37 +0000 (+0200) Subject: [5565] Updated CalloutHandleStore to store multiple hanles. X-Git-Tag: trac5549a_base~32^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=cfd2632e2a2a1b39a83a50affcfae5af317dee9f;p=thirdparty%2Fkea.git [5565] Updated CalloutHandleStore to store multiple hanles. --- diff --git a/src/lib/dhcpsrv/callout_handle_store.h b/src/lib/dhcpsrv/callout_handle_store.h index a35a2213ad..43d8d56cb7 100644 --- a/src/lib/dhcpsrv/callout_handle_store.h +++ b/src/lib/dhcpsrv/callout_handle_store.h @@ -1,4 +1,4 @@ -// Copyright (C) 2013-2015 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-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 @@ -9,6 +9,8 @@ #include #include +#include +#include namespace isc { namespace dhcp { @@ -19,24 +21,30 @@ namespace dhcp { /// isc::hooks::CalloutHandle object with each request passing through the /// server. For the DHCP servers, the association is provided by this function. /// -/// The DHCP servers process a single request at a time. 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 is stored, a new CalloutHandle is allocated (and stored) and -/// a pointer to the latter object returned to the caller. If the request -/// matches the one stored, the pointer to the stored CalloutHandle is -/// returned. +/// 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. /// -/// A special case is a null pointer being passed. This has the effect of -/// clearing the stored pointers to the packet being processed and -/// CalloutHandle. As the stored pointers are shared pointers, clearing them -/// removes one reference that keeps the pointed-to objects in existence. +/// 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. /// -/// @note If the behaviour of the server changes so that multiple packets can -/// be active at the same time, this simplistic approach will no longer -/// be adequate and a more complicated structure (such as a map) will -/// be needed. /// +/// 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. +/// +/// @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. @@ -45,33 +53,49 @@ namespace dhcp { /// 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 /// pointer is returned if pktptr is itself an empty pointer. - template isc::hooks::CalloutHandlePtr getCalloutHandle(const T& pktptr) { - // Stored data is declared static, so is initialized when first accessed - static T stored_pointer; // Pointer to last packet seen - static isc::hooks::CalloutHandlePtr stored_handle; - // Pointer to stored handle + isc::hooks::CalloutHandlePtr stored_handle; + static std::map store; if (pktptr) { - // Pointer given, have we seen it before? (If we have, we don't need to - // do anything as we will automatically return the stored handle.) - if (pktptr != stored_pointer) { + 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. (The latter operation frees and probably deletes - // (depending on other pointers) the stored one.) - stored_pointer = pktptr; + // 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 - stored_pointer.reset(); - stored_handle.reset(); + store.clear(); } return (stored_handle); diff --git a/src/lib/dhcpsrv/tests/callout_handle_store_unittest.cc b/src/lib/dhcpsrv/tests/callout_handle_store_unittest.cc index a22aa61530..c7eef28684 100644 --- a/src/lib/dhcpsrv/tests/callout_handle_store_unittest.cc +++ b/src/lib/dhcpsrv/tests/callout_handle_store_unittest.cc @@ -59,7 +59,7 @@ TEST(CalloutHandleStoreTest, StoreRetrieve) { 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] + // 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()); @@ -73,9 +73,23 @@ TEST(CalloutHandleStoreTest, StoreRetrieve) { EXPECT_FALSE(chptr_1 == chptr_2); // Check reference counts. The getCalloutHandle function should be storing - // pointers to the objects pointed to by chptr_2 and pktptr_2. + // 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(1, pktptr_1.use_count()); EXPECT_EQ(2, chptr_2.use_count()); EXPECT_EQ(2, pktptr_2.use_count()); @@ -89,7 +103,6 @@ TEST(CalloutHandleStoreTest, StoreRetrieve) { // Reference counts should be back to 1 for the CalloutHandles and the // Packet pointers. EXPECT_EQ(1, chptr_1.use_count()); - EXPECT_EQ(1, pktptr_1.use_count()); EXPECT_EQ(1, chptr_2.use_count()); EXPECT_EQ(1, pktptr_2.use_count()); }