[#890] make allocation engine thread safe

src/lib/dhcpsrv/alloc_engine.cc

@@ -34,18 +34,20 @@
 
 #include <algorithm>
 #include <cstring>
-#include <sstream>
 #include <limits>
-#include <vector>
+#include <sstream>
 #include <stdint.h>
 #include <string.h>
 #include <utility>
+#include <vector>
+
 
 using namespace isc::asiolink;
 using namespace isc::dhcp;
 using namespace isc::dhcp_ddns;
 using namespace isc::hooks;
 using namespace isc::stats;
+using namespace isc::util;
 
 namespace {
 
@@ -87,7 +89,7 @@ namespace isc {
 namespace dhcp {
 
 AllocEngine::IterativeAllocator::IterativeAllocator(Lease::Type lease_type)
-    :Allocator(lease_type) {
+    : Allocator(lease_type) {
 }
 
 isc::asiolink::IOAddress
@@ -106,17 +108,14 @@ AllocEngine::IterativeAllocator::increasePrefix(const isc::asiolink::IOAddress&
                   << prefix_len);
     }
 
-    // Brief explanation what happens here:
-    // http://www.youtube.com/watch?v=NFQCYpIHLNQ
-
     uint8_t n_bytes = (prefix_len - 1)/8;
     uint8_t n_bits = 8 - (prefix_len - n_bytes*8);
     uint8_t mask = 1 << n_bits;
 
-    // Longer explanation: n_bytes specifies number of full bytes that are
+    // Explanation: n_bytes specifies number of full bytes that are in-prefix.
-    // in-prefix. They can also be used as an offset for the first byte that
+    // They can also be used as an offset for the first byte that is not in
-    // is not in prefix. n_bits specifies number of bits on the last byte that
+    // prefix. n_bits specifies number of bits on the last byte that is
-    // is (often partially) in prefix. For example for a /125 prefix, the values
+    // (often partially) in prefix. For example for a /125 prefix, the values
     // are 15 and 3, respectively. Mask is a bitmask that has the least
     // significant bit from the prefix set.
 
@@ -158,11 +157,10 @@ AllocEngine::IterativeAllocator::increaseAddress(const isc::asiolink::IOAddress&
 }
 
 isc::asiolink::IOAddress
-AllocEngine::IterativeAllocator::pickAddress(const SubnetPtr& subnet,
+AllocEngine::IterativeAllocator::pickAddressInternal(const SubnetPtr& subnet,
-                                             const ClientClasses& client_classes,
+                                                     const ClientClasses& client_classes,
-                                             const DuidPtr&,
+                                                     const DuidPtr&,
-                                             const IOAddress&) {
+                                                     const IOAddress&) {
-
     // Is this prefix allocation?
     bool prefix = pool_type_ == Lease::TYPE_PD;
     uint8_t prefix_len = 0;
@@ -287,28 +285,28 @@ AllocEngine::IterativeAllocator::pickAddress(const SubnetPtr& subnet,
 }
 
 AllocEngine::HashedAllocator::HashedAllocator(Lease::Type lease_type)
-    :Allocator(lease_type) {
+    : Allocator(lease_type) {
     isc_throw(NotImplemented, "Hashed allocator is not implemented");
 }
 
 isc::asiolink::IOAddress
-AllocEngine::HashedAllocator::pickAddress(const SubnetPtr&,
+AllocEngine::HashedAllocator::pickAddressInternal(const SubnetPtr&,
-                                          const ClientClasses&,
+                                                  const ClientClasses&,
-                                          const DuidPtr&,
+                                                  const DuidPtr&,
-                                          const IOAddress&) {
+                                                  const IOAddress&) {
     isc_throw(NotImplemented, "Hashed allocator is not implemented");
 }
 
 AllocEngine::RandomAllocator::RandomAllocator(Lease::Type lease_type)
-    :Allocator(lease_type) {
+    : Allocator(lease_type) {
     isc_throw(NotImplemented, "Random allocator is not implemented");
 }
 
 isc::asiolink::IOAddress
-AllocEngine::RandomAllocator::pickAddress(const SubnetPtr&,
+AllocEngine::RandomAllocator::pickAddressInternal(const SubnetPtr&,
-                                          const ClientClasses&,
+                                                  const ClientClasses&,
-                                          const DuidPtr&,
+                                                  const DuidPtr&,
-                                          const IOAddress&) {
+                                                  const IOAddress&) {
     isc_throw(NotImplemented, "Random allocator is not implemented");
 }
 
@@ -1058,7 +1056,7 @@ AllocEngine::allocateUnreservedLeases6(ClientContext6& ctx) {
             }
 
             Lease6Ptr existing = LeaseMgrFactory::instance().getLease6(ctx.currentIA().type_,
-                                                                   candidate);
+                                                                       candidate);
             if (!existing) {
 
                 // there's no existing lease for selected candidate, so it is
@@ -3375,9 +3373,6 @@ AllocEngine::requestLease4(AllocEngine::ClientContext4& ctx) {
     Lease4Ptr client_lease;
     findClientLease(ctx, client_lease);
 
-    // Obtain the sole instance of the LeaseMgr.
-    LeaseMgr& lease_mgr = LeaseMgrFactory::instance();
-
     // When the client sends the DHCPREQUEST, it should always specify the
     // address which it is requesting or renewing. That is, the client should
     // either use the requested IP address option or set the ciaddr. However,
@@ -3539,7 +3534,7 @@ AllocEngine::requestLease4(AllocEngine::ClientContext4& ctx) {
             .arg(ctx.query_->getLabel())
             .arg(client_lease->addr_.toText());
 
-        if (lease_mgr.deleteLease(client_lease)) {
+        if (LeaseMgrFactory::instance().deleteLease(client_lease)) {
             // Need to decrease statistic for assigned addresses.
             StatsMgr::instance().addValue(
                 StatsMgr::generateName("subnet", client_lease->subnet_id_,
@@ -3583,14 +3578,12 @@ AllocEngine::createLease4(const ClientContext4& ctx, const IOAddress& addr,
 
     time_t now = time(NULL);
 
-    // @todo: remove this kludge?
+    ClientIdPtr client_id;
-    std::vector<uint8_t> local_copy;
+    if (ctx.subnet_->getMatchClientId()) {
-    if (ctx.clientid_ && ctx.subnet_->getMatchClientId()) {
+        client_id = ctx.clientid_;
-        local_copy = ctx.clientid_->getDuid();
     }
-    const uint8_t* local_copy0 = local_copy.empty() ? 0 : &local_copy[0];
 
-    Lease4Ptr lease(new Lease4(addr, ctx.hwaddr_, local_copy0, local_copy.size(),
+    Lease4Ptr lease(new Lease4(addr, ctx.hwaddr_, client_id,
                                valid_lft, now, ctx.subnet_->getID()));
 
     // Set FQDN specific lease parameters.

src/lib/dhcpsrv/alloc_engine.h

@@ -22,6 +22,7 @@
 #include <dhcpsrv/lease_mgr.h>
 #include <dhcpsrv/srv_config.h>
 #include <hooks/callout_handle.h>
+#include <util/multi_threading_mgr.h>
 
 #include <boost/function.hpp>
 #include <boost/shared_ptr.hpp>
@@ -29,6 +30,7 @@
 
 #include <list>
 #include <map>
+#include <mutex>
 #include <set>
 #include <utility>
 
@@ -88,30 +90,44 @@ protected:
         /// @param subnet next address will be returned from pool of that subnet
         /// @param client_classes list of classes client belongs to
         /// @param duid Client's DUID
-        /// @param hint client's hint
+        /// @param hint Client's hint
         ///
         /// @return the next address
         virtual isc::asiolink::IOAddress
         pickAddress(const SubnetPtr& subnet,
                     const ClientClasses& client_classes,
                     const DuidPtr& duid,
-                    const isc::asiolink::IOAddress& hint) = 0;
+                    const isc::asiolink::IOAddress& hint) {
+            return util::MultiThreadingMgr::call(
+                mutex_, [&]() {return pickAddressInternal(subnet, client_classes, duid, hint);});
+        }
 
         /// @brief Default constructor.
         ///
         /// Specifies which type of leases this allocator will assign
         /// @param pool_type specifies pool type (addresses, temp. addr or prefixes)
-        Allocator(Lease::Type pool_type)
+        Allocator(Lease::Type pool_type) : pool_type_(pool_type) {
-            :pool_type_(pool_type) {
         }
 
         /// @brief virtual destructor
         virtual ~Allocator() {
         }
+
+    private:
+        virtual isc::asiolink::IOAddress
+        pickAddressInternal(const SubnetPtr& subnet,
+                            const ClientClasses& client_classes,
+                            const DuidPtr& duid,
+                            const isc::asiolink::IOAddress& hint) = 0;
+
     protected:
 
         /// @brief defines pool type allocation
         Lease::Type pool_type_;
+
+    private:
+
+        std::mutex mutex_;
     };
 
     /// defines a pointer to allocator
@@ -132,18 +148,22 @@ protected:
         /// @param type - specifies allocation type
         IterativeAllocator(Lease::Type type);
 
+    private:
+
         /// @brief returns the next address from pools in a subnet
         ///
         /// @param subnet next address will be returned from pool of that subnet
         /// @param client_classes list of classes client belongs to
         /// @param duid Client's DUID (ignored)
-        /// @param hint client's hint (ignored)
+        /// @param hint Client's hint (ignored)
+        ///
         /// @return the next address
         virtual isc::asiolink::IOAddress
-            pickAddress(const SubnetPtr& subnet,
+        pickAddressInternal(const SubnetPtr& subnet,
-                        const ClientClasses& client_classes,
+                            const ClientClasses& client_classes,
-                        const DuidPtr& duid,
+                            const DuidPtr& duid,
-                        const isc::asiolink::IOAddress& hint);
+                            const isc::asiolink::IOAddress& hint);
+
     protected:
 
         /// @brief Returns the next prefix
@@ -155,6 +175,7 @@ protected:
         ///
         /// @param prefix prefix to be increased
         /// @param prefix_len length of the prefix to be increased
+        ///
         /// @return result prefix
         static isc::asiolink::IOAddress
         increasePrefix(const isc::asiolink::IOAddress& prefix,
@@ -168,11 +189,11 @@ protected:
         /// @param address address or prefix to be increased
         /// @param prefix true when the previous argument is a prefix
         /// @param prefix_len length of the prefix
+        ///
         /// @return result address or prefix
         static isc::asiolink::IOAddress
         increaseAddress(const isc::asiolink::IOAddress& address,
                         bool prefix, const uint8_t prefix_len);
-
     };
 
     /// @brief Address/prefix allocator that gets an address based on a hash
@@ -185,6 +206,8 @@ protected:
         /// @param type - specifies allocation type
         HashedAllocator(Lease::Type type);
 
+    private:
+
         /// @brief returns an address based on hash calculated from client's DUID.
         ///
         /// @todo: Implement this method
@@ -193,12 +216,13 @@ protected:
         /// @param client_classes list of classes client belongs to
         /// @param duid Client's DUID
         /// @param hint a hint (last address that was picked)
+        ///
         /// @return selected address
         virtual isc::asiolink::IOAddress
-            pickAddress(const SubnetPtr& subnet,
+        pickAddressInternal(const SubnetPtr& subnet,
-                        const ClientClasses& client_classes,
+                            const ClientClasses& client_classes,
-                        const DuidPtr& duid,
+                            const DuidPtr& duid,
-                        const isc::asiolink::IOAddress& hint);
+                            const isc::asiolink::IOAddress& hint);
     };
 
     /// @brief Random allocator that picks address randomly
@@ -211,6 +235,8 @@ protected:
         /// @param type - specifies allocation type
         RandomAllocator(Lease::Type type);
 
+    private:
+
         /// @brief returns a random address from pool of specified subnet
         ///
         /// @todo: Implement this method
@@ -219,12 +245,13 @@ protected:
         /// @param client_classes list of classes client belongs to
         /// @param duid Client's DUID (ignored)
         /// @param hint the last address that was picked (ignored)
+        ///
         /// @return a random address from the pool
         virtual isc::asiolink::IOAddress
-        pickAddress(const SubnetPtr& subnet,
+        pickAddressInternal(const SubnetPtr& subnet,
-                    const ClientClasses& client_classes,
+                            const ClientClasses& client_classes,
-                    const DuidPtr& duid,
+                            const DuidPtr& duid,
-                    const isc::asiolink::IOAddress& hint);
+                            const isc::asiolink::IOAddress& hint);
     };
 
 public:
@@ -255,7 +282,9 @@ public:
     /// @brief Returns allocator for a given pool type
     ///
     /// @param type type of pool (V4, IA, TA or PD)
+    ///
     /// @throw BadValue if allocator for a given type is missing
+    ///
     /// @return pointer to allocator handling a given resource types
     AllocatorPtr getAllocator(Lease::Type type);
 
@@ -334,6 +363,7 @@ public:
         /// @brief Compares two @c AllocEngine::Resource objects for equality.
         ///
         /// @param other object to be compared with this object
+        ///
         /// @return true if objects are equal, false otherwise.
         bool equals(const Resource& other) const {
             return (address_ == other.address_ &&
@@ -343,6 +373,7 @@ public:
         /// @brief Equality operator.
         ///
         /// @param other object to be compared with this object
+        ///
         /// @return true if objects are equal, false otherwise.
         bool operator==(const Resource& other) const {
             return (equals(other));
@@ -370,8 +401,9 @@ public:
         /// @brief Compare operator
         ///
         /// @note Only the address/prefix part of resources is used.
-        /// @param lhr Left hand resource objet
+        /// @param lhr Left hand resource object
-        /// @param rhr Right hand resource objet
+        /// @param rhr Right hand resource object
+        ///
         /// @return true if lhr is less than rhr, false otherwise
         bool operator() (const Resource& lhr, const Resource& rhr) const {
             if (lhr.getAddress() == rhr.getAddress()) {
@@ -541,12 +573,14 @@ public:
             /// @brief Convenience method adding new hint from IAADDR option.
             ///
             /// @param iaaddr Pointer to IAADDR.
+            ///
             /// @throw BadValue if iaaddr is null.
             void addHint(const Option6IAAddrPtr& iaaddr);
 
             /// @brief Convenience method adding new hint from IAPREFIX option.
             ///
             /// @param iaprefix Pointer to IAPREFIX.
+            ///
             /// @throw BadValue if iaprefix is null.
             void addHint(const Option6IAPrefixPtr& iaprefix);
         };
@@ -908,12 +942,14 @@ public:
     /// pointer if no matches are found.
     ///
     /// @param ctx Client context holding various information about the client.
+    ///
     /// @return Pointer to the reservation found, or an empty pointer.
     static ConstHostPtr findGlobalReservation(ClientContext6& ctx);
 
     /// @brief Creates an IPv6Resrv instance from a Lease6
     ///
     /// @param lease Reference to the Lease6
+    ///
     /// @return The newly formed IPv6Resrv instance
     static IPv6Resrv makeIPv6Resrv(const Lease6& lease) {
         if (lease.type_ == Lease::TYPE_NA) {
@@ -941,10 +977,10 @@ private:
     /// @param [out] callout_status callout returned by the lease6_select
     ///
     /// The following fields of the ctx structure are used:
-    /// @ref ClientContext6::subnet_ subnet the lease is allocated from
+    /// @ref ClientContext6::subnet_ Subnet the lease is allocated from
-    /// @ref ClientContext6::duid_ client's DUID
+    /// @ref ClientContext6::duid_ Client's DUID
     /// @ref ClientContext6::iaid_ IAID from the IA_NA container the client sent to us
-    /// @ref ClientContext6::type_ lease type (IA, TA or PD)
+    /// @ref ClientContext6::type_ Lease type (IA, TA or PD)
     /// @ref ClientContext6::fwd_dns_update_ A boolean value which indicates that server takes
     ///        responsibility for the forward DNS Update for this lease
     ///        (if true).
@@ -958,6 +994,7 @@ private:
     ///        registered)
     /// @ref ClientContext6::fake_allocation_ is this real i.e. REQUEST (false) or just picking
     ///        an address for SOLICIT that is not really allocated (true)
+    ///
     /// @return allocated lease (or NULL in the unlikely case of the lease just
     ///         became unavailable)
     Lease6Ptr createLease6(ClientContext6& ctx,
@@ -973,6 +1010,7 @@ private:
     /// This may change in the future.
     ///
     /// @param ctx client context that contains all details (subnet, client-id, etc.)
+    ///
     /// @return collection of newly allocated leases
     Lease6Collection allocateUnreservedLeases6(ClientContext6& ctx);
 
@@ -1061,8 +1099,8 @@ private:
     /// @param [out] callout_status callout returned by the lease6_select
     ///
     /// The following parameters are used from the ctx structure:
-    /// @ref ClientContext6::subnet_ subnet the lease is allocated from
+    /// @ref ClientContext6::subnet_ Subnet the lease is allocated from
-    /// @ref ClientContext6::duid_ client's DUID
+    /// @ref ClientContext6::duid_ Client's DUID
     /// @ref ClientContext6::iaid_ IAID from the IA_NA container the client sent to us
     /// @ref ClientContext6::fwd_dns_update_ A boolean value which indicates that server takes
     ///        responsibility for the forward DNS Update for this lease
@@ -1078,6 +1116,7 @@ private:
     ///        allocated (true)
     ///
     /// @return refreshed lease
+    ///
     /// @throw BadValue if trying to recycle lease that is still valid
     Lease6Ptr
     reuseExpiredLease(Lease6Ptr& expired,
@@ -1109,6 +1148,7 @@ private:
     /// @brief Utility function that removes all leases with a specified address
     /// @param container A collection of Lease6 pointers
     /// @param addr address to be removed
+    ///
     /// @return true if removed (false otherwise)
     static bool
     removeLeases(Lease6Collection& container,
@@ -1232,6 +1272,7 @@ private:
     /// - call lease4_recover hook
     ///
     /// @param lease Lease to be reclaimed from Declined state
+    ///
     /// @return true if it's ok to remove the lease (false = hooks status says
     ///         to keep it)
     bool reclaimDeclined(const Lease4Ptr& lease);
@@ -1245,6 +1286,7 @@ private:
     /// - call lease6_recover hook
     ///
     /// @param lease Lease to be reclaimed from Declined state
+    ///
     /// @return true if it's ok to remove the lease (false = hooks status says
     ///         to keep it)
     bool reclaimDeclined(const Lease6Ptr& lease);
@@ -1518,6 +1560,7 @@ public:
     /// matches are found.
     ///
     /// @param ctx Client context holding various information about the client.
+    ///
     /// @return Pointer to the reservation found, or an empty pointer.
     static ConstHostPtr findGlobalReservation(ClientContext4& ctx);
 
@@ -1585,7 +1628,7 @@ private:
     /// -# If the client is not requesting any specific address, allocate
     ///    the address from the dynamic pool.
     ///
-    /// @throws various exceptions if the allocation goes wrong.
+    /// @throw various exceptions if the allocation goes wrong.
     ///
     /// @param ctx Client context holding the data extracted from the
     /// client's message.
@@ -1620,6 +1663,7 @@ private:
     /// - @ref ClientContext4::fake_allocation_ Is this real i.e. REQUEST (false)
     ///        or just picking an address for DISCOVER that is not really
     ///        allocated (true)
+    ///
     /// @return allocated lease (or NULL in the unlikely case of the lease just
     ///        become unavailable)
     Lease4Ptr createLease4(const ClientContext4& ctx,
@@ -1654,6 +1698,7 @@ private:
     /// @param [out] callout_status callout returned by the lease4_select
     ///
     /// @return Updated lease instance.
+    ///
     /// @throw BadValue if trying to reuse a lease which is still valid or
     /// when the provided parameters are invalid.
     Lease4Ptr

src/lib/util/multi_threading_mgr.h

@@ -8,6 +8,7 @@
 #define MULTI_THREADING_MGR_H
 
 #include <boost/noncopyable.hpp>
+#include <mutex>
 
 namespace isc {
 namespace util {
@@ -28,18 +29,14 @@ namespace util {
 /// For instance for a class protected by its mutex:
 /// @code
 /// namespace locked {
-///     void foo() { ... }
+///     int foo() { ... }
 /// } // end of locked namespace
 ///
-/// void foo() {
+/// int foo() {
-///     if (MultiThreadingMgr::instance().getMode()) {
+///     return MultiThreadingMgr::call(mutex_, []() {return locked::foo()});
-///         lock_guard<mutex> lock(mutex_);
-///         locked::foo();
-///     } else {
-///         locked::foo();
-///     }
 /// }
 /// @endcode
+
 class MultiThreadingMgr : public boost::noncopyable {
 public:
 
@@ -62,6 +59,23 @@ public:
     /// @param enabled The new mode.
     void setMode(bool enabled);
 
+    /// @brief Call a Functor in MT or ST mode
+    ///
+    /// @tparam Lockable a lock which is used to create a thread safe context
+    /// @tparam Callable a functor which will be called in MT or ST mode
+    /// @param lk the lock object to perform lock in MT mode
+    /// @param f the functor to call
+    /// @result the result of the functor call
+    template<typename Lockable, typename Callable>
+    static auto call(Lockable& lk, const Callable& f) -> decltype(f()) {
+        if (MultiThreadingMgr::instance().getMode()) {
+            std::lock_guard<Lockable> lock(lk);
+            return f();
+        } else {
+            return f();
+        }
+    }
+
 protected:
 
     /// @brief Constructor.