From: Razvan Becheriu Date: Mon, 15 Jun 2020 13:10:24 +0000 (+0300) Subject: [#1239] addressed comments X-Git-Tag: Kea-1.7.9~55 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=96745e24aa9ad48b3f8d0003ffe9c999f1f8a8ae;p=thirdparty%2Fkea.git [#1239] addressed comments --- diff --git a/src/hooks/dhcp/high_availability/communication_state.cc b/src/hooks/dhcp/high_availability/communication_state.cc index d3898d7627..d495adc983 100644 --- a/src/hooks/dhcp/high_availability/communication_state.cc +++ b/src/hooks/dhcp/high_availability/communication_state.cc @@ -71,17 +71,12 @@ void CommunicationState::modifyPokeTime(const long secs) { if (MultiThreadingMgr::instance().getMode()) { std::lock_guard lk(*mutex_); - modifyPokeTimeInternal(secs); + poke_time_ += boost::posix_time::seconds(secs); } else { - modifyPokeTimeInternal(secs); + poke_time_ += boost::posix_time::seconds(secs); } } -void -CommunicationState::modifyPokeTimeInternal(const long secs) { - poke_time_ += boost::posix_time::seconds(secs); -} - void CommunicationState::setPartnerState(const std::string& state) { try { @@ -443,12 +438,13 @@ CommunicationState4::analyzeMessageInternal(const boost::shared_ptr& // Only log the first time we detect a client is unacked. if (log_unacked) { unsigned unacked_left = 0; - if (config_->getMaxUnackedClients() > getUnackedClientsCountInternal()) { - unacked_left = config_->getMaxUnackedClients() - getUnackedClientsCountInternal(); + unsigned unacked_total = connecting_clients_.get<1>().count(true); + if (config_->getMaxUnackedClients() > unacked_total) { + unacked_left = config_->getMaxUnackedClients() - unacked_total; } LOG_INFO(ha_logger, HA_COMMUNICATION_INTERRUPTED_CLIENT4_UNACKED) .arg(message->getLabel()) - .arg(getUnackedClientsCountInternal()) + .arg(unacked_total) .arg(unacked_left); } } @@ -466,54 +462,40 @@ CommunicationState4::failureDetected() const { bool CommunicationState4::failureDetectedInternal() const { return ((config_->getMaxUnackedClients() == 0) || - (getUnackedClientsCountInternal() > config_->getMaxUnackedClients())); + (connecting_clients_.get<1>().count(true) > + config_->getMaxUnackedClients())); } size_t CommunicationState4::getConnectingClientsCount() const { if (MultiThreadingMgr::instance().getMode()) { std::lock_guard lk(*mutex_); - return (getConnectingClientsCountInternal()); + return (connecting_clients_.size()); } else { - return (getConnectingClientsCountInternal()); + return (connecting_clients_.size()); } } -size_t -CommunicationState4::getConnectingClientsCountInternal() const { - return (connecting_clients_.size()); -} - size_t CommunicationState4::getUnackedClientsCount() const { if (MultiThreadingMgr::instance().getMode()) { std::lock_guard lk(*mutex_); - return (getUnackedClientsCountInternal()); + return (connecting_clients_.get<1>().count(true)); } else { - return (getUnackedClientsCountInternal()); + return (connecting_clients_.get<1>().count(true)); } } -size_t -CommunicationState4::getUnackedClientsCountInternal() const { - return (connecting_clients_.get<1>().count(true)); -} - void CommunicationState4::clearConnectingClients() { if (MultiThreadingMgr::instance().getMode()) { std::lock_guard lk(*mutex_); - clearConnectingClientsInternal(); + connecting_clients_.clear(); } else { - clearConnectingClientsInternal(); + connecting_clients_.clear(); } } -void -CommunicationState4::clearConnectingClientsInternal() { - connecting_clients_.clear(); -} - CommunicationState6::CommunicationState6(const IOServicePtr& io_service, const HAConfigPtr& config) : CommunicationState(io_service, config), connecting_clients_(), @@ -588,12 +570,13 @@ CommunicationState6::analyzeMessageInternal(const boost::shared_ptr& // Only log the first time we detect a client is unacked. if (log_unacked) { unsigned unacked_left = 0; - if (config_->getMaxUnackedClients() > getUnackedClientsCountInternal()) { - unacked_left = config_->getMaxUnackedClients() - getUnackedClientsCountInternal(); + unsigned unacked_total = connecting_clients_.get<1>().count(true); + if (config_->getMaxUnackedClients() > unacked_total) { + unacked_left = config_->getMaxUnackedClients() - unacked_total; } LOG_INFO(ha_logger, HA_COMMUNICATION_INTERRUPTED_CLIENT6_UNACKED) .arg(message->getLabel()) - .arg(getUnackedClientsCountInternal()) + .arg(unacked_total) .arg(unacked_left); } } @@ -611,53 +594,39 @@ CommunicationState6::failureDetected() const { bool CommunicationState6::failureDetectedInternal() const { return ((config_->getMaxUnackedClients() == 0) || - (getUnackedClientsCountInternal() > config_->getMaxUnackedClients())); + (connecting_clients_.get<1>().count(true) > + config_->getMaxUnackedClients())); } size_t CommunicationState6::getConnectingClientsCount() const { if (MultiThreadingMgr::instance().getMode()) { std::lock_guard lk(*mutex_); - return (getConnectingClientsCountInternal()); + return (connecting_clients_.size()); } else { - return (getConnectingClientsCountInternal()); + return (connecting_clients_.size()); } } -size_t -CommunicationState6::getConnectingClientsCountInternal() const { - return (connecting_clients_.size()); -} - size_t CommunicationState6::getUnackedClientsCount() const { if (MultiThreadingMgr::instance().getMode()) { std::lock_guard lk(*mutex_); - return (getUnackedClientsCountInternal()); + return (connecting_clients_.get<1>().count(true)); } else { - return (getUnackedClientsCountInternal()); + return (connecting_clients_.get<1>().count(true)); } } -size_t -CommunicationState6::getUnackedClientsCountInternal() const { - return (connecting_clients_.get<1>().count(true)); -} - void CommunicationState6::clearConnectingClients() { if (MultiThreadingMgr::instance().getMode()) { std::lock_guard lk(*mutex_); - clearConnectingClientsInternal(); + connecting_clients_.clear(); } else { - clearConnectingClientsInternal(); + connecting_clients_.clear(); } } -void -CommunicationState6::clearConnectingClientsInternal() { - connecting_clients_.clear(); -} - } // end of namespace isc::ha } // end of namespace isc diff --git a/src/hooks/dhcp/high_availability/communication_state.h b/src/hooks/dhcp/high_availability/communication_state.h index 985854f7b7..5549a20c24 100644 --- a/src/hooks/dhcp/high_availability/communication_state.h +++ b/src/hooks/dhcp/high_availability/communication_state.h @@ -359,17 +359,6 @@ public: 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); - /// @brief Returns duration between the poke time and current time. /// /// Should be called in a thread safe context. @@ -526,30 +515,6 @@ protected: /// otherwise. virtual bool failureDetectedInternal() const; - /// @brief Returns the current number of clients which attempted - /// to get a lease from the partner server. - /// - /// Should be called in a thread safe context. - /// - /// The returned number is reset to 0 when the server successfully - /// establishes communication with the partner. The number is - /// incremented only in the communications interrupted case. - /// - /// @return The number of clients including unacked clients. - virtual size_t getConnectingClientsCountInternal() const; - - /// @brief Returns the current number of clients which haven't gotten - /// a lease from the partner server. - /// - /// Should be called in a thread safe context. - /// - /// The returned number is reset to 0 when the server successfully - /// establishes communication with the partner. The number is - /// incremented only in the communications interrupted case. - /// - /// @return Number of unacked clients. - virtual size_t getUnackedClientsCountInternal() const; - /// @brief Removes information about the clients the partner server /// should respond to while communication with the partner was /// interrupted. @@ -557,15 +522,6 @@ protected: /// See @c CommunicationState::analyzeMessage for details. virtual void clearConnectingClients(); - /// @brief Removes information about the clients the partner server - /// should respond to while communication with the partner was - /// interrupted. - /// - /// Should be called in a thread safe context. - /// - /// See @c CommunicationState::analyzeMessage for details. - virtual void clearConnectingClientsInternal(); - /// @brief Structure holding information about the client which has /// send the packet being analyzed. struct ConnectingClient4 { @@ -685,30 +641,6 @@ protected: /// otherwise. virtual bool failureDetectedInternal() const; - /// @brief Returns the current number of clients which attempted - /// to get a lease from the partner server. - /// - /// Should be called in a thread safe context. - /// - /// The returned number is reset to 0 when the server successfully - /// establishes communication with the partner. The number is - /// incremented only in the communications interrupted case. - /// - /// @return The number of clients including unacked clients. - virtual size_t getConnectingClientsCountInternal() const; - - /// @brief Returns the current number of clients which haven't gotten - /// a lease from the partner server. - /// - /// Should be called in a thread safe context. - /// - /// The returned number is reset to 0 when the server successfully - /// establishes communication with the partner. The number is - /// incremented only in the communications interrupted case. - /// - /// @return Number of unacked clients. - virtual size_t getUnackedClientsCountInternal() const; - /// @brief Removes information about the clients the partner server /// should respond to while communication with the partner was /// interrupted. @@ -716,15 +648,6 @@ protected: /// See @c CommunicationState::analyzeMessage for details. virtual void clearConnectingClients(); - /// @brief Removes information about the clients the partner server - /// should respond to while communication with the partner was - /// interrupted. - /// - /// Should be called in a thread safe context. - /// - /// See @c CommunicationState::analyzeMessage for details. - virtual void clearConnectingClientsInternal(); - /// @brief Structure holding information about a client which /// sent a packet being analyzed. struct ConnectingClient6 { diff --git a/src/lib/dhcpsrv/csv_lease_file4.cc b/src/lib/dhcpsrv/csv_lease_file4.cc index 0336be7933..1fb8d19ca1 100644 --- a/src/lib/dhcpsrv/csv_lease_file4.cc +++ b/src/lib/dhcpsrv/csv_lease_file4.cc @@ -5,10 +5,7 @@ // file, You can obtain one at http://mozilla.org/MPL/2.0/. #include - #include -#include - #include using namespace isc::asiolink; @@ -25,16 +22,6 @@ 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); @@ -42,34 +29,8 @@ CSVLeaseFile4::openInternal(const bool seek_to_end) { clearStatistics(); } -void -CSVLeaseFile4::close() { - if (MultiThreadingMgr::instance().getMode()) { - std::lock_guard lock(mutex_); - closeInternal(); - } else { - closeInternal(); - } -} - -void -CSVLeaseFile4::closeInternal() { - // Call the base class to close the file - VersionedCSVFile::close(); -} - 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_; @@ -124,16 +85,6 @@ CSVLeaseFile4::appendInternal(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 93f09f95d3..67df846536 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 capability of @c Lease4. +/// validation capablity of @c Lease4. class CSVLeaseFile4 : public isc::util::VersionedCSVFile, public LeaseFileStats { public: @@ -50,11 +50,6 @@ public: /// the base class may do so. virtual void open(const bool seek_to_end = false); - /// @brief Closes the lease file. - /// - /// This function should hide instead of overwrite the base class function. - virtual void close(); - /// @brief Appends the lease record to the CSV file. /// /// This function doesn't throw exceptions itself. In theory, exceptions @@ -92,61 +87,6 @@ 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 Closes the lease file. - /// - /// Should be called in a thread safe context. - virtual void closeInternal(); - - /// @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: @@ -224,8 +164,6 @@ 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 2b32d56891..cf2f0c0c5c 100644 --- a/src/lib/dhcpsrv/csv_lease_file6.cc +++ b/src/lib/dhcpsrv/csv_lease_file6.cc @@ -5,11 +5,8 @@ // file, You can obtain one at http://mozilla.org/MPL/2.0/. #include - #include #include -#include - #include using namespace isc::asiolink; @@ -26,32 +23,6 @@ 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::close() { - if (MultiThreadingMgr::instance().getMode()) { - std::lock_guard lock(mutex_); - closeInternal(); - } else { - closeInternal(); - } -} - -void -CSVLeaseFile6::closeInternal() { - // Call the base class to close the file - VersionedCSVFile::close(); -} - -void -CSVLeaseFile6::openInternal(const bool seek_to_end) { // Call the base class to open the file VersionedCSVFile::open(seek_to_end); @@ -61,16 +32,6 @@ CSVLeaseFile6::openInternal(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_; @@ -118,16 +79,6 @@ CSVLeaseFile6::appendInternal(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 2cbc52cb58..84acbd0a85 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 capability of @c Lease6. +/// validation capablity of @c Lease6. class CSVLeaseFile6 : public isc::util::VersionedCSVFile, public LeaseFileStats { public: @@ -49,11 +49,6 @@ public: /// the base class may do so. virtual void open(const bool seek_to_end = false); - /// @brief Closes the lease file. - /// - /// This function should hide instead of overwrite the base class function. - virtual void close(); - /// @brief Appends the lease record to the CSV file. /// /// This function doesn't throw exceptions itself. In theory, exceptions @@ -62,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 not - /// in STATE_DECLINED. + /// @throw BadValue if the lease to be written has an empty DUID and is + /// whose state is not STATE_DECLINED. void append(const Lease6& lease); /// @brief Reads next lease from the CSV file. @@ -88,58 +83,6 @@ 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 Closes the lease file. - /// - /// Should be called in a thread safe context. - virtual void closeInternal(); - - /// @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: @@ -242,8 +185,6 @@ 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/memfile_lease_mgr.cc b/src/lib/dhcpsrv/memfile_lease_mgr.cc index 278fb671b9..9b8a3afe36 100644 --- a/src/lib/dhcpsrv/memfile_lease_mgr.cc +++ b/src/lib/dhcpsrv/memfile_lease_mgr.cc @@ -1846,9 +1846,10 @@ Memfile_LeaseMgr::lfcCallback() { // Check if we're in the v4 or v6 space and use the appropriate file. if (lease_file4_) { + MultiThreadingCriticalSection cs; lfcExecute(lease_file4_); - } else if (lease_file6_) { + MultiThreadingCriticalSection cs; lfcExecute(lease_file6_); } } diff --git a/src/lib/dhcpsrv/timer_mgr.h b/src/lib/dhcpsrv/timer_mgr.h index eaef7cafcb..cdb95887ea 100644 --- a/src/lib/dhcpsrv/timer_mgr.h +++ b/src/lib/dhcpsrv/timer_mgr.h @@ -51,6 +51,12 @@ typedef boost::shared_ptr TimerMgrPtr; /// Before the @c TimerMgr can be used the server process must call /// @c TimerMgr::setIOService to associate the manager with the IO service /// that the server is using to its run tasks. +/// +/// @note Only scheduling new timer (calling @ref setup) and canceling existing +/// timer (calling @ref cancel) are thread safe. +/// Registering new timers (calling @ref registerTimer) and unregistering +/// existing timers (calling @ref unregisterTimer) must be handled before +/// starting processing threads. class TimerMgr : public boost::noncopyable { public: diff --git a/src/lib/http/client.cc b/src/lib/http/client.cc index 6d777ad61e..b991f31be7 100644 --- a/src/lib/http/client.cc +++ b/src/lib/http/client.cc @@ -514,22 +514,13 @@ public: private: - /// @brief Returns next queued request for the given URL. + /// @brief Process next queued request for the given URL. /// /// This method should be called in a thread safe context. /// /// @param url URL for which next queued request should be retrieved. - /// @param [out] request Pointer to the queued request. - /// @param [out] response Pointer to the object into which response should - /// be stored. - /// @param request_timeout Requested timeout for the transaction. - /// @param callback Pointer to the user callback for this request. - /// @param connect_callback Pointer to the user callback invoked when - /// the client connects to the server. - /// @param close_callback Pointer to the user callback invoked when - /// the client closes the connection to the server. - /// - /// @return true if the request for the given URL has been retrieved, + /// + /// @return true if the request for the given URL has been processed, /// false if there are no more requests queued for this URL. bool processNextRequestInternal(const Url& url) { // Check if there is a queue for this URL. If there is no queue, there @@ -691,6 +682,7 @@ private: struct RequestDescriptor { /// @brief Constructor. /// + /// @param conn Pointer to the connection. /// @param request Pointer to the request to be sent. /// @param response Pointer to the object into which the response will /// be stored. @@ -764,7 +756,6 @@ Connection::resetState() { current_callback_ = HttpClient::RequestHandler(); } - void Connection::closeCallback(const bool clear) { if (close_callback_) {