From: Thomas Markwalder Date: Wed, 31 Mar 2021 20:14:26 +0000 (-0400) Subject: [#1733] Parking lot enchancements X-Git-Tag: Kea-1.9.7~83 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f2ffdb5b68eb4f22d4f3791f02393199a3ca9e66;p=thirdparty%2Fkea.git [#1733] Parking lot enchancements src/lib/hooks/parking_lots.h ParkingLot now stores objects in a map instead of list to eliminate sequential searches ParkingLotHandle::park() - now allows parking without a pre-existing reference ParkingLotHandle::deference() - new method that decrements parked object reference counts without invoking their callback src/lib/hooks/tests/parking_lots_unittest.cc TEST(ParkingLotsTest, parkRequireReferenceTests) TEST(ParkingLotTest, dereference) TEST(ParkingLotTest, multipleObjects) - new tests --- diff --git a/src/lib/hooks/parking_lots.h b/src/lib/hooks/parking_lots.h index 4c78d2826c..e50c7b8bdd 100644 --- a/src/lib/hooks/parking_lots.h +++ b/src/lib/hooks/parking_lots.h @@ -1,4 +1,4 @@ -// Copyright (C) 2018-2020 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2018-2021 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 @@ -14,9 +14,11 @@ #include #include +#include #include #include #include +#include namespace isc { namespace hooks { @@ -60,26 +62,45 @@ namespace hooks { /// destruction of the parked objects. class ParkingLot { public: - /// @brief Parks an object. /// /// @tparam Type of the parked object. /// @param parked_object object to be parked, e.g. pointer to a packet. /// @param unpark_callback callback function to be invoked when the object /// is unparked. - /// @throw InvalidOperation if the @c reference() wasn't called prior to - /// parking the object. + /// @param require_reference when true, the object to park must have an + /// existing reference in the parking lot. If false, the reference will be + /// be created. + /// @throw InvalidOperation if the @c reference() or @c park() have not been + /// called prior to parking the object. + /// @throw Unexpected if the reference count is not greater than zero. This + /// represents an invalid state and should not happen. template - void park(T parked_object, std::function unpark_callback) { + void park(T parked_object, std::function unpark_callback, + bool require_reference = true) { std::lock_guard lock(mutex_); auto it = find(parked_object); - if (it == parking_.end() || it->refcount_ <= 0) { - isc_throw(InvalidOperation, "unable to park an object because" - " reference count for this object hasn't been increased." - " Call ParkingLot::reference() first"); - } else { - it->update(parked_object, unpark_callback); + if (it == parking_.end()) { + if (require_reference) { + isc_throw(InvalidOperation, "unable to park an object because" + " reference does not exist."); + } + + // Not there add it. + ParkingInfo pinfo(parked_object, unpark_callback); + parking_[makeKey(parked_object)] = pinfo; + return; + } + + // Catch the case where it exists but refcount is invalid. + if (it->second.refcount_ <= 0) { + isc_throw(Unexpected, "unable to park an object because" + " reference hasn't been increased."); } + + // Already there, bump the reference count and set the + // the callback. + it->second.update(parked_object, unpark_callback); } /// @brief Increases reference counter for the parked object. @@ -95,11 +116,43 @@ public: std::lock_guard lock(mutex_); auto it = find(parked_object); if (it == parking_.end()) { - ParkingInfo parking_info(parked_object); - parking_.push_back(parking_info); + // It's not there, add it with a ref count of 1. + ParkingInfo pinfo(parked_object); + parking_[makeKey(parked_object)] = pinfo; } else { - ++it->refcount_; + // It's already there, bump the ref count. + ++it->second.refcount_; + } + } + + /// @brief Decreases the reference counter for the parked object. + /// + /// This method is called by the callouts to decrease the reference count + /// on a parked object. If the reference count reaches zero the object + /// is removed from the parking lot without invoking its callback. + /// + /// @tparam Type of the parked object. + /// @param parked_object object which will be parked. + /// @return true if the object was found in the parking lot, false + /// otherwise. + template + bool dereference(T parked_object) { + std::lock_guard lock(mutex_); + auto it = find(parked_object); + if (it == parking_.end()) { + // It's not there, nothing to do. + return (false); + } + + // It's there decrement the ref count. + --it->second.refcount_; + + if (it->second.refcount_ <= 0) { + // No more references, unpark it. + parking_.erase(it); } + + return (true); } /// @brief Signals that the object should be unparked. @@ -128,14 +181,14 @@ public: } if (force) { - it->refcount_ = 0; + it->second.refcount_ = 0; } else { - --it->refcount_; + --it->second.refcount_; } - if (it->refcount_ <= 0) { + if (it->second.refcount_ <= 0) { // Unpark the packet and set the callback. - cb = it->unpark_callback_; + cb = it->second.unpark_callback_; parking_.erase(it); } } @@ -172,7 +225,7 @@ public: return (false); } -private: +public: /// @brief Holds information about parked object. struct ParkingInfo { @@ -180,6 +233,11 @@ private: std::function unpark_callback_; ///< pointer to the callback int refcount_; ///< current reference count + /// @brief Constructor. + /// + /// Default constructor. + ParkingInfo() {}; + /// @brief Constructor. /// /// @param parked_object object being parked. @@ -201,8 +259,10 @@ private: } }; - /// @brief Type of list of parked objects. - typedef std::list ParkingInfoList; +private: + + /// @brief Map which stores parked objects. + typedef std::map ParkingInfoList; /// @brief Type of the iterator in the list of parked objects. typedef ParkingInfoList::iterator ParkingInfoListIterator; @@ -210,20 +270,27 @@ private: /// @brief Container holding parked objects for this parking lot. ParkingInfoList parking_; + /// @brief Construct the key for a given parked object. + /// + /// @tparam T parked object type. + /// @param parked_object object from which the key should be constructed. + /// @return string containing the object's key. + template + std::string makeKey(T parked_object) { + std::stringstream ss; + ss << boost::any_cast(parked_object); + return(ss.str()); + } + /// @brief Search for the information about the parked object. /// /// @tparam T parked object type. - /// @param parked_object object for which the information should be found. - /// @return Iterator pointing to the parked object, or @c parking_.end() if - /// no such object found. + /// @param parked_object object for which to search. + /// @return Iterator pointing to the parked object, or @c parking_.end() + /// if no such object found. template ParkingInfoListIterator find(T parked_object) { - for (auto it = parking_.begin(); it != parking_.end(); ++it) { - if (boost::any_cast(it->parked_object_) == parked_object) { - return (it); - } - } - return (parking_.end()); + return(parking_.find(makeKey(parked_object))); } /// @brief The mutex to protect parking lot internal state. @@ -270,6 +337,21 @@ public: parking_lot_->reference(parked_object); } + /// @brief Decreases the reference counter for the parked object. + /// + /// This method is called by the callouts to decrease the reference count + /// of a parked object. If the reference count reaches zero the object + /// is removed from the parking lot without invoking its callback. + /// + /// @tparam Type of the parked object. + /// @param parked_object object which will be parked. + /// @return true if the object was found in the parking lot, false + /// otherwise. + template + bool dereference(T parked_object) { + return (parking_lot_->dereference(parked_object)); + } + /// @brief Signals that the object should be unparked. /// /// If the specified object is parked in this parking lot, the reference diff --git a/src/lib/hooks/tests/parking_lots_unittest.cc b/src/lib/hooks/tests/parking_lots_unittest.cc index 62d0d49c71..a42789351f 100644 --- a/src/lib/hooks/tests/parking_lots_unittest.cc +++ b/src/lib/hooks/tests/parking_lots_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2018-2020 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2018-2021 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,7 @@ #include #include #include +#include #include #include @@ -17,6 +18,12 @@ using namespace isc::hooks; namespace { +// Defines a pointer to a string. The tests use shared pointers +// as parked objects to ensure key matching works correctly with +// them. We're doing this because real-world use parked objects +// are typically pointers to packets. +typedef boost::shared_ptr StringPtr; + // Test that it is possible to create and retrieve parking lots for // specified hook points. TEST(ParkingLotsTest, createGetParkingLot) { @@ -41,22 +48,34 @@ TEST(ParkingLotsTest, createGetParkingLot) { EXPECT_FALSE(parking_lot3 == parking_lot0); } -// Test that object can't be parked if it hasn't been referenced on the -// parking lot. -TEST(ParkingLotTest, parkWithoutReferencing) { +// Verify that parking objects obey require-reference parameter. +TEST(ParkingLotsTest, parkRequireReferenceTests) { ParkingLot parking_lot; - std::string parked_object = "foo"; - EXPECT_THROW(parking_lot.park(parked_object, [] { - }), InvalidOperation); + + // Create object to park. + StringPtr parked_object(new std::string("foo")); + + // Cannot park an unreferenced object by default + EXPECT_THROW(parking_lot.park(parked_object, [] {}), + InvalidOperation); + + // Cannot park an unreferenced object when require_reference + // parameter is true. + EXPECT_THROW(parking_lot.park(parked_object, [] {}, true), + InvalidOperation); + + // Can park an unreferenced object when require_reference + // parameter is false. + EXPECT_NO_THROW(parking_lot.park(parked_object, [] {}, false)); } // Test that object can be parked and then unparked. -TEST(ParkingLotTest, unpark) { +TEST(ParkingLotsTest, unpark) { ParkingLotPtr parking_lot = boost::make_shared(); ParkingLotHandlePtr parking_lot_handle = boost::make_shared(parking_lot); - std::string parked_object = "foo"; + StringPtr parked_object(new std::string("foo")); // Reference the parked object twice because we're going to test that // reference counting works fine. @@ -85,12 +104,12 @@ TEST(ParkingLotTest, unpark) { } // Test that parked object can be dropped. -TEST(ParkingLotTest, drop) { +TEST(ParkingLotsTest, drop) { ParkingLotPtr parking_lot = boost::make_shared(); ParkingLotHandlePtr parking_lot_handle = boost::make_shared(parking_lot); - std::string parked_object = "foo"; + StringPtr parked_object(new std::string("foo")); // Reference object twice to test that dropping the packet ignores // reference counting. @@ -113,7 +132,7 @@ TEST(ParkingLotTest, drop) { } // Test that parked lots can be cleared. -TEST(ParkingLotTest, clear) { +TEST(ParkingLotsTest, clear) { ParkingLotsPtr parking_lots = boost::make_shared(); ParkingLotPtr parking_lot = parking_lots->getParkingLotPtr(1234); ASSERT_TRUE(parking_lot); @@ -153,4 +172,82 @@ TEST(ParkingLotTest, clear) { EXPECT_TRUE(weak_parked_object.expired()); } +// Test that object can be parked and dereferenced. +TEST(ParkingLotsTest, dereference) { + ParkingLotPtr parking_lot = boost::make_shared(); + ParkingLotHandlePtr parking_lot_handle = + boost::make_shared(parking_lot); + + StringPtr parked_object(new std::string("foo")); + + // Reference the parked object twice because we're going to test that + // reference counting works fine. + ASSERT_NO_THROW(parking_lot_handle->reference(parked_object)); + ASSERT_NO_THROW(parking_lot_handle->reference(parked_object)); + + // This flag will indicate if the callback has been called. + bool unparked = false; + ASSERT_NO_THROW(parking_lot->park(parked_object, [&unparked] { + unparked = true; + })); + + // Try to dereference the object. It should decrease the reference count, + // but not unpark the packet yet or invoke the callback. + EXPECT_TRUE(parking_lot_handle->dereference(parked_object)); + EXPECT_FALSE(unparked); + + // Try to dereference the object. It should decrease the reference count, + // but and unpark the packet but not invoke the callback. + EXPECT_TRUE(parking_lot_handle->dereference(parked_object)); + EXPECT_FALSE(unparked); + + // Try to dereference the object. It should return false indicating + // the object is no longer parked. + EXPECT_FALSE(parking_lot_handle->dereference(parked_object)); + EXPECT_FALSE(unparked); +} + +// Verifies that parked objects are correctly distinguished from +// one another. +TEST(ParkingLotsTest, multipleObjects) { + ParkingLotPtr parking_lot = boost::make_shared(); + ParkingLotHandlePtr parking_lot_handle = + boost::make_shared(parking_lot); + + // Create an object and park it. + StringPtr object_one(new std::string("one")); + int unparked_one = 0; + ASSERT_NO_THROW(parking_lot->park(object_one, [&unparked_one] { + ++unparked_one; + }, false)); + + // Create a second object and park it. + StringPtr object_two(new std::string("two")); + int unparked_two = 0; + ASSERT_NO_THROW(parking_lot->park(object_two, [&unparked_two] { + ++unparked_two; + }, false)); + + // Create a third object but don't park it. + StringPtr object_three(new std::string("three")); + + // Try to unpark object_three. It should fail, and no callbacks + // should get invoked. + EXPECT_FALSE(parking_lot_handle->unpark(object_three)); + EXPECT_EQ(unparked_one, 0); + EXPECT_EQ(unparked_two, 0); + + // Unpark object one. It should succeed and its callback should + // get invoked. + EXPECT_TRUE(parking_lot_handle->unpark(object_one)); + EXPECT_EQ(unparked_one, 1); + EXPECT_EQ(unparked_two, 0); + + // Unpark object two. It should succeed and its callback should + // get invoked. + EXPECT_TRUE(parking_lot_handle->unpark(object_two)); + EXPECT_EQ(unparked_one, 1); + EXPECT_EQ(unparked_two, 1); +} + }