From: Razvan Becheriu Date: Fri, 5 Jun 2020 08:13:29 +0000 (+0300) Subject: [#1239] csv lease file is now kea thread safe X-Git-Tag: Kea-1.7.9~59 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=083b1c624bea4497cf8a65bbbe9bb3f8b5f5b983;p=thirdparty%2Fkea.git [#1239] csv lease file is now kea thread safe --- diff --git a/src/hooks/dhcp/high_availability/communication_state.cc b/src/hooks/dhcp/high_availability/communication_state.cc index 80f82e6352..d3898d7627 100644 --- a/src/hooks/dhcp/high_availability/communication_state.cc +++ b/src/hooks/dhcp/high_availability/communication_state.cc @@ -517,7 +517,7 @@ CommunicationState4::clearConnectingClientsInternal() { CommunicationState6::CommunicationState6(const IOServicePtr& io_service, const HAConfigPtr& config) : CommunicationState(io_service, config), connecting_clients_(), - mutex_(new mutex()){ + mutex_(new mutex()) { } void diff --git a/src/hooks/dhcp/high_availability/communication_state.h b/src/hooks/dhcp/high_availability/communication_state.h index fc0e0b8761..985854f7b7 100644 --- a/src/hooks/dhcp/high_availability/communication_state.h +++ b/src/hooks/dhcp/high_availability/communication_state.h @@ -352,26 +352,28 @@ public: /// /// Used in unittests only. /// - /// Should be called in a thread safe context. - /// /// @param secs number of seconds to be added to the poke time. If /// the value is negative it will set the poke time in the past /// comparing to current value. void modifyPokeTime(const long secs); +protected: + /// @brief Modifies poke time by adding seconds to it. /// /// Used in unittests only. /// + /// Should be called in a thread safe context. + /// /// @param secs number of seconds to be added to the poke time. If /// the value is negative it will set the poke time in the past /// comparing to current value. void modifyPokeTimeInternal(const long secs); -protected: - /// @brief Returns duration between the poke time and current time. /// + /// Should be called in a thread safe context. + /// /// @return Duration between the poke time and current time. int64_t getDurationInMillisecsInternal() const; diff --git a/src/lib/dhcpsrv/csv_lease_file4.cc b/src/lib/dhcpsrv/csv_lease_file4.cc index 1fb8d19ca1..a66da20321 100644 --- a/src/lib/dhcpsrv/csv_lease_file4.cc +++ b/src/lib/dhcpsrv/csv_lease_file4.cc @@ -5,7 +5,10 @@ // file, You can obtain one at http://mozilla.org/MPL/2.0/. #include + #include +#include + #include using namespace isc::asiolink; @@ -22,6 +25,16 @@ CSVLeaseFile4::CSVLeaseFile4(const std::string& filename) void CSVLeaseFile4::open(const bool seek_to_end) { + if (MultiThreadingMgr::instance().getMode()) { + std::lock_guard lock(mutex_); + openInternal(seek_to_end); + } else { + openInternal(seek_to_end); + } +} + +void +CSVLeaseFile4::openInternal(const bool seek_to_end) { // Call the base class to open the file VersionedCSVFile::open(seek_to_end); @@ -31,6 +44,16 @@ CSVLeaseFile4::open(const bool seek_to_end) { void CSVLeaseFile4::append(const Lease4& lease) { + if (MultiThreadingMgr::instance().getMode()) { + std::lock_guard lock(mutex_); + appendInternal(lease); + } else { + appendInternal(lease); + } +} + +void +CSVLeaseFile4::appendInternal(const Lease4& lease) { // Bump the number of write attempts ++writes_; @@ -85,6 +108,16 @@ CSVLeaseFile4::append(const Lease4& lease) { bool CSVLeaseFile4::next(Lease4Ptr& lease) { + if (MultiThreadingMgr::instance().getMode()) { + std::lock_guard lock(mutex_); + return (nextInternal(lease)); + } else { + return (nextInternal(lease)); + } +} + +bool +CSVLeaseFile4::nextInternal(Lease4Ptr& lease) { // Bump the number of read attempts ++reads_; diff --git a/src/lib/dhcpsrv/csv_lease_file4.h b/src/lib/dhcpsrv/csv_lease_file4.h index 67df846536..477adf7561 100644 --- a/src/lib/dhcpsrv/csv_lease_file4.h +++ b/src/lib/dhcpsrv/csv_lease_file4.h @@ -30,7 +30,7 @@ namespace dhcp { /// The @c Lease4 is a structure that should be itself responsible for this /// validation (see http://oldkea.isc.org/ticket/2405). However, when #2405 /// is implemented, the @c next function may need to be updated to use the -/// validation capablity of @c Lease4. +/// validation capability of @c Lease4. class CSVLeaseFile4 : public isc::util::VersionedCSVFile, public LeaseFileStats { public: @@ -87,6 +87,56 @@ public: private: + /// @brief Opens a lease file. + /// + /// Should be called in a thread safe context. + /// + /// This function calls the base class open to do the + /// work of opening a file. It is used to clear any + /// statistics associated with any previous use of the file + /// While it doesn't throw any exceptions of its own + /// the base class may do so. + virtual void openInternal(const bool seek_to_end = false); + + /// @brief Appends the lease record to the CSV file. + /// + /// Should be called in a thread safe context. + /// + /// This function doesn't throw exceptions itself. In theory, exceptions + /// are possible when the index of the indexes of the values being written + /// to the file are invalid. However, this would have been a programming + /// error. + /// + /// @param lease Structure representing a DHCPv4 lease. + /// @throw BadValue if the lease has no hardware address, no client id and + /// is not in STATE_DECLINED. + void appendInternal(const Lease4& lease); + + /// @brief Reads next lease from the CSV file. + /// + /// Should be called in a thread safe context. + /// + /// If this function hits an error during lease read, it sets the error + /// message using @c CSVFile::setReadMsg and returns false. The error + /// string may be read using @c CSVFile::getReadMsg. + /// + /// Treats rows without a hardware address or a client id when their + /// state is not STATE_DECLINED as an error. + /// + /// This function is exception safe. + /// + /// @param [out] lease Pointer to the lease read from CSV file or + /// NULL pointer if lease hasn't been read. + /// + /// @return Boolean value indicating that the new lease has been + /// read from the CSV file (if true), or that the error has occurred + /// (false). + /// + /// @todo Make sure that the values read from the file are correct. + /// The appropriate @c Lease4 validation mechanism should be used once + /// ticket http://oldkea.isc.org/ticket/2405 is implemented. + bool nextInternal(Lease4Ptr& lease); + /// @brief Initializes columns of the CSV file holding leases. /// /// This function initializes the following columns: @@ -164,6 +214,8 @@ private: data::ConstElementPtr readContext(const util::CSVRow& row); //@} + /// @brief Mutex to protect the internal state. + std::mutex mutex_; }; } // namespace isc::dhcp diff --git a/src/lib/dhcpsrv/csv_lease_file6.cc b/src/lib/dhcpsrv/csv_lease_file6.cc index cf2f0c0c5c..cb8ea894c8 100644 --- a/src/lib/dhcpsrv/csv_lease_file6.cc +++ b/src/lib/dhcpsrv/csv_lease_file6.cc @@ -5,8 +5,11 @@ // file, You can obtain one at http://mozilla.org/MPL/2.0/. #include + #include #include +#include + #include using namespace isc::asiolink; @@ -23,6 +26,16 @@ CSVLeaseFile6::CSVLeaseFile6(const std::string& filename) void CSVLeaseFile6::open(const bool seek_to_end) { + if (MultiThreadingMgr::instance().getMode()) { + std::lock_guard lock(mutex_); + openInternal(seek_to_end); + } else { + openInternal(seek_to_end); + } +} + +void +CSVLeaseFile6::openInternal(const bool seek_to_end) { // Call the base class to open the file VersionedCSVFile::open(seek_to_end); @@ -32,6 +45,16 @@ CSVLeaseFile6::open(const bool seek_to_end) { void CSVLeaseFile6::append(const Lease6& lease) { + if (MultiThreadingMgr::instance().getMode()) { + std::lock_guard lock(mutex_); + appendInternal(lease); + } else { + appendInternal(lease); + } +} + +void +CSVLeaseFile6::appendInternal(const Lease6& lease) { // Bump the number of write attempts ++writes_; @@ -79,6 +102,16 @@ CSVLeaseFile6::append(const Lease6& lease) { bool CSVLeaseFile6::next(Lease6Ptr& lease) { + if (MultiThreadingMgr::instance().getMode()) { + std::lock_guard lock(mutex_); + return (nextInternal(lease)); + } else { + return (nextInternal(lease)); + } +} + +bool +CSVLeaseFile6::nextInternal(Lease6Ptr& lease) { // Bump the number of read attempts ++reads_; diff --git a/src/lib/dhcpsrv/csv_lease_file6.h b/src/lib/dhcpsrv/csv_lease_file6.h index 84acbd0a85..41cc949efd 100644 --- a/src/lib/dhcpsrv/csv_lease_file6.h +++ b/src/lib/dhcpsrv/csv_lease_file6.h @@ -29,7 +29,7 @@ namespace dhcp { /// The @c Lease6 is a structure that should be itself responsible for this /// validation (see http://oldkea.isc.org/ticket/2405). However, when #2405 /// is implemented, the @c next function may need to be updated to use the -/// validation capablity of @c Lease6. +/// validation capability of @c Lease6. class CSVLeaseFile6 : public isc::util::VersionedCSVFile, public LeaseFileStats { public: @@ -57,8 +57,8 @@ public: /// error. /// /// @param lease Structure representing a DHCPv6 lease. - /// @throw BadValue if the lease to be written has an empty DUID and is - /// whose state is not STATE_DECLINED. + /// @throw BadValue if the lease to be written has an empty DUID and is not + /// in STATE_DECLINED. void append(const Lease6& lease); /// @brief Reads next lease from the CSV file. @@ -83,6 +83,53 @@ public: private: + /// @brief Opens a lease file. + /// + /// Should be called in a thread safe context. + /// + /// This function calls the base class open to do the + /// work of opening a file. It is used to clear any + /// statistics associated with any previous use of the file + /// While it doesn't throw any exceptions of its own + /// the base class may do so. + virtual void openInternal(const bool seek_to_end = false); + + /// @brief Appends the lease record to the CSV file. + /// + /// Should be called in a thread safe context. + /// + /// This function doesn't throw exceptions itself. In theory, exceptions + /// are possible when the index of the indexes of the values being written + /// to the file are invalid. However, this would have been a programming + /// error. + /// + /// @param lease Structure representing a DHCPv6 lease. + /// @throw BadValue if the lease to be written has an empty DUID and is not + /// in STATE_DECLINED. + void appendInternal(const Lease6& lease); + + /// @brief Reads next lease from the CSV file. + /// + /// Should be called in a thread safe context. + /// + /// If this function hits an error during lease read, it sets the error + /// message using @c CSVFile::setReadMsg and returns false. The error + /// string may be read using @c CSVFile::getReadMsg. + /// + /// This function is exception safe. + /// + /// @param [out] lease Pointer to the lease read from CSV file or + /// NULL pointer if lease hasn't been read. + /// + /// @return Boolean value indicating that the new lease has been + /// read from the CSV file (if true), or that the error has occurred + /// (false). + /// + /// @todo Make sure that the values read from the file are correct. + /// The appropriate @c Lease6 validation mechanism should be used once + /// ticket http://oldkea.isc.org/ticket/2405 is implemented. + bool nextInternal(Lease6Ptr& lease); + /// @brief Initializes columns of the CSV file holding leases. /// /// This function initializes the following columns: @@ -185,6 +232,8 @@ private: data::ConstElementPtr readContext(const util::CSVRow& row); //@} + /// @brief Mutex to protect the internal state. + std::mutex mutex_; }; } // namespace isc::dhcp diff --git a/src/lib/http/client.cc b/src/lib/http/client.cc index 5fc9d63f09..6d777ad61e 100644 --- a/src/lib/http/client.cc +++ b/src/lib/http/client.cc @@ -971,6 +971,11 @@ Connection::terminateInternal(const boost::system::error_code& ec, } } + // unlock mutex so that the callback can be safely processed. + if (MultiThreadingMgr::instance().getMode()) { + mutex_.unlock(); + } + try { // The callback should take care of its own exceptions but one // never knows. @@ -979,6 +984,11 @@ Connection::terminateInternal(const boost::system::error_code& ec, } catch (...) { } + // lock mutex so that processing can continue. + if (MultiThreadingMgr::instance().getMode()) { + mutex_.lock(); + } + // If we're not requesting connection persistence, we should close the socket. // We're going to reconnect for the next transaction. if (!current_request_->isPersistent()) {