From: JINMEI Tatuya Date: Sat, 20 Oct 2012 05:27:30 +0000 (-0700) Subject: [2211] use DataSrcClientsMgr for query processing X-Git-Tag: trac2402_base~3^2~14 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=86fe8fce9db80466c67e0553c9f5ef404cdbb185;p=thirdparty%2Fkea.git [2211] use DataSrcClientsMgr for query processing --- diff --git a/src/bin/auth/auth_srv.cc b/src/bin/auth/auth_srv.cc index f90b116028..e27098258e 100644 --- a/src/bin/auth/auth_srv.cc +++ b/src/bin/auth/auth_srv.cc @@ -654,15 +654,15 @@ AuthSrvImpl::processNormalQuery(const IOMessage& io_message, Message& message, local_edns->setUDPSize(AuthSrvImpl::DEFAULT_LOCAL_UDPSIZE); message.setEDNS(local_edns); } - // Lock the client lists and keep them under the lock until the processing - // and rendering is done (this is the same mutex as from - // AuthSrv::getDataSrcClientListMutex()). - isc::util::thread::Mutex::Locker locker(mutex_); + // Get access to data source client list through the holder and keep thek + // holder until the processing and rendering is done to avoid inter-thread + // race. + auth::DataSrcClientsMgr::Holder datasrc_holder(datasrc_clients_mgr_); try { const ConstQuestionPtr question = *message.beginQuestion(); const shared_ptr - list(getDataSrcClientList(question->getClass())); + list(datasrc_holder.findClientList(question->getClass())); if (list) { const RRType& qtype = question->getType(); const Name& qname = question->getName(); diff --git a/src/bin/auth/command.cc b/src/bin/auth/command.cc index 6ee616870c..6b8b0ce267 100644 --- a/src/bin/auth/command.cc +++ b/src/bin/auth/command.cc @@ -192,9 +192,10 @@ public: const Name origin(origin_elem->stringValue()); DataSrcClientsMgr::Holder holder(server.getDataSrcClientsMgr()); - ConfigurableClientList* list = holder.findClientList(zone_class); + boost::shared_ptr list = + holder.findClientList(zone_class); - if (list == NULL) { + if (!list) { isc_throw(AuthCommandError, "There's no client list for " "class " << zone_class); } diff --git a/src/bin/auth/datasrc_clients_mgr.h b/src/bin/auth/datasrc_clients_mgr.h index 937365b0d0..01c346f43b 100644 --- a/src/bin/auth/datasrc_clients_mgr.h +++ b/src/bin/auth/datasrc_clients_mgr.h @@ -116,15 +116,15 @@ public: /// otherwise it returns NULL. The manager keeps the ownership of /// the pointed object. Also, it's not safe to get access to the /// object beyond the scope of the holder object. - datasrc::ConfigurableClientList* findClientList( + boost::shared_ptr findClientList( const dns::RRClass& rrclass) { const ClientListsMap::const_iterator it = mgr_.clients_map_->find(rrclass); if (it == mgr_.clients_map_->end()) { - return (NULL); + return (boost::shared_ptr()); } else { - return (it->second.get()); + return (it->second); } } private: diff --git a/src/bin/auth/tests/auth_srv_unittest.cc b/src/bin/auth/tests/auth_srv_unittest.cc index a202c13fb6..0781d578b5 100644 --- a/src/bin/auth/tests/auth_srv_unittest.cc +++ b/src/bin/auth/tests/auth_srv_unittest.cc @@ -70,6 +70,7 @@ using namespace isc::util::unittests; using namespace isc::dns::rdata; using namespace isc::data; using namespace isc::xfr; +using namespace isc::auth; using namespace isc::asiodns; using namespace isc::asiolink; using namespace isc::testutils; @@ -726,11 +727,11 @@ TEST_F(AuthSrvTest, notifyWithSessionMessageError) { } void -installDataSrcClientLists(AuthSrv& server, - DataSrcClientListsPtr lists) -{ - thread::Mutex::Locker locker(server.getDataSrcClientListMutex()); - server.swapDataSrcClientLists(lists); +installDataSrcClientLists(AuthSrv& server, DataSrcClientListsPtr lists) { + // For now, we use explicit swap than reconfigure() because the latter + // involves a separate thread and cannot guarantee the new config is + // available for the subsequent test. + server.getDataSrcClientsMgr().swapDataSrcClientLists(lists); } void @@ -1438,16 +1439,16 @@ TEST_F(AuthSrvTest, { // Set real inmem client to proxy updateInMemory(server, "example.", CONFIG_INMEMORY_EXAMPLE); + boost::shared_ptr list; + DataSrcClientsMgr& mgr = server.getDataSrcClientsMgr(); { - isc::util::thread::Mutex::Locker locker( - server.getDataSrcClientListMutex()); - boost::shared_ptr - list(new FakeList(server.getDataSrcClientList(RRClass::IN()), - THROW_NEVER, false)); - DataSrcClientListsPtr lists(new std::map); - lists->insert(pair(RRClass::IN(), list)); - server.swapDataSrcClientLists(lists); + DataSrcClientsMgr::Holder holder(mgr); + list.reset(new FakeList(holder.findClientList(RRClass::IN()), + THROW_NEVER, false)); } + DataSrcClientListsPtr lists(new std::map); + lists->insert(pair(RRClass::IN(), list)); + server.getDataSrcClientsMgr().swapDataSrcClientLists(lists); createDataFromFile("nsec3query_nodnssec_fromWire.wire"); server.processMessage(*io_message, *parse_message, *response_obuffer, @@ -1470,14 +1471,16 @@ setupThrow(AuthSrv& server, ThrowWhen throw_when, bool isc_exception, { updateInMemory(server, "example.", CONFIG_INMEMORY_EXAMPLE); - isc::util::thread::Mutex::Locker locker( - server.getDataSrcClientListMutex()); - boost::shared_ptr - list(new FakeList(server.getDataSrcClientList(RRClass::IN()), - throw_when, isc_exception, rrset)); + boost::shared_ptr list; + DataSrcClientsMgr& mgr = server.getDataSrcClientsMgr(); + { // we need to limit the scope so swap is outside of it + DataSrcClientsMgr::Holder holder(mgr); + list.reset(new FakeList(holder.findClientList(RRClass::IN()), + throw_when, isc_exception, rrset)); + } DataSrcClientListsPtr lists(new std::map); lists->insert(pair(RRClass::IN(), list)); - server.swapDataSrcClientLists(lists); + mgr.swapDataSrcClientLists(lists); } TEST_F(AuthSrvTest, diff --git a/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc b/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc index 1258a04c67..7dbee06d4a 100644 --- a/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc +++ b/src/bin/auth/tests/datasrc_clients_mgr_unittest.cc @@ -154,10 +154,8 @@ TEST(DataSrcClientsMgrTest, holder) { { // Initially it's empty, so findClientList() will always return NULL TestDataSrcClientsMgr::Holder holder(mgr); - EXPECT_EQ(static_cast(NULL), - holder.findClientList(RRClass::IN())); - EXPECT_EQ(static_cast(NULL), - holder.findClientList(RRClass::CH())); + EXPECT_FALSE(holder.findClientList(RRClass::IN())); + EXPECT_FALSE(holder.findClientList(RRClass::CH())); // map should be protected here EXPECT_EQ(1, FakeDataSrcClientsBuilder::map_mutex->lock_count); EXPECT_EQ(0, FakeDataSrcClientsBuilder::map_mutex->unlock_count); @@ -174,10 +172,8 @@ TEST(DataSrcClientsMgrTest, holder) { mgr.reconfigure(reconfigure_arg); { TestDataSrcClientsMgr::Holder holder(mgr); - EXPECT_NE(static_cast(NULL), - holder.findClientList(RRClass::IN())); - EXPECT_NE(static_cast(NULL), - holder.findClientList(RRClass::CH())); + EXPECT_TRUE(holder.findClientList(RRClass::IN())); + EXPECT_TRUE(holder.findClientList(RRClass::CH())); } // We need to clear command queue by hand FakeDataSrcClientsBuilder::command_queue->clear(); @@ -190,10 +186,8 @@ TEST(DataSrcClientsMgrTest, holder) { mgr.reconfigure(reconfigure_arg); { TestDataSrcClientsMgr::Holder holder(mgr); - EXPECT_NE(static_cast(NULL), - holder.findClientList(RRClass::IN())); - EXPECT_EQ(static_cast(NULL), - holder.findClientList(RRClass::CH())); + EXPECT_TRUE(holder.findClientList(RRClass::IN())); + EXPECT_FALSE(holder.findClientList(RRClass::CH())); } }