[master] Merge branch 'trac2459'

src/bin/auth/auth_messages.mes

@@ -98,6 +98,16 @@ This debug message is issued when the separate thread for maintaining data
 source clients successfully loaded the named zone of the named class as a
 result of the 'loadzone' command.
 
+% AUTH_DATASRC_CLIENTS_BUILDER_LOAD_ZONE_NOCACHE skipped loading zone %1/%2 due to no in-memory cache
+This debug message is issued when the separate thread for maintaining data
+source clients received a command to reload a zone but skipped it because
+the specified zone is not loaded in-memory (but served from an underlying
+data source).  This could happen if the loadzone command is manually issued
+by a user but the zone name is misspelled, but a more likely cause is
+that the command is sent from another BIND 10 module (such as xfrin or DDNS).
+In the latter case it can be simply ignored because there is no need
+for explicit reloading.
+
 % AUTH_DATASRC_CLIENTS_BUILDER_RECONFIGURE_CONFIG_ERROR Error in data source configuration: %1
 The thread for maintaining data source clients has received a command to
 reconfigure, but the parameter data (the new configuration) contains an

src/bin/auth/auth_srv.cc

@@ -651,9 +651,10 @@ AuthSrvImpl::processNormalQuery(const IOMessage& io_message, Message& message,
         local_edns->setUDPSize(AuthSrvImpl::DEFAULT_LOCAL_UDPSIZE);
         message.setEDNS(local_edns);
     }
-    // Get access to data source client list through the holder and keep the
-    // holder until the processing and rendering is done to avoid inter-thread
-    // race.
+
+    // Get access to data source client list through the holder and keep
+    // the holder until the processing and rendering is done to avoid
+    // race with any other thread(s) such as the background loader.
     auth::DataSrcClientsMgr::Holder datasrc_holder(datasrc_clients_mgr_);
 
     try {
@@ -688,6 +689,9 @@ AuthSrvImpl::processNormalQuery(const IOMessage& io_message, Message& message,
     return (true);
     // The message can contain some data from the locked resource. But outside
     // this method, we touch only the RCode of it, so it should be safe.
+
+    // Lock on datasrc_clients_mgr_ acquired by datasrc_holder is
+    // released here upon its deletion.
 }
 
 bool

src/bin/auth/datasrc_clients_mgr.h

@@ -581,6 +581,9 @@ DataSrcClientsBuilderBase<MutexType, CondVarType>::doLoadZone(
     try {
         boost::shared_ptr<datasrc::memory::ZoneWriter> zwriter =
             getZoneWriter(*client_list, rrclass, origin);
+        if (!zwriter) {
+            return;
+        }
 
         zwriter->load(); // this can take time but doesn't cause a race
         {   // install() can cause a race and must be in a critical section
@@ -614,8 +617,14 @@ DataSrcClientsBuilderBase<MutexType, CondVarType>::getZoneWriter(
     datasrc::ConfigurableClientList& client_list,
     const dns::RRClass& rrclass, const dns::Name& origin)
 {
-    const datasrc::ConfigurableClientList::ZoneWriterPair writerpair =
-        client_list.getCachedZoneWriter(origin);
+    // getCachedZoneWriter() could get access to an underlying data source
+    // that can cause a race condition with the main thread using that data
+    // source for lookup.  So we need to protect the access here.
+    datasrc::ConfigurableClientList::ZoneWriterPair writerpair;
+    {
+        typename MutexType::Locker locker(*map_mutex_);
+        writerpair = client_list.getCachedZoneWriter(origin);
+    }
 
     switch (writerpair.first) {
     case datasrc::ConfigurableClientList::ZONE_SUCCESS:
@@ -626,8 +635,10 @@ DataSrcClientsBuilderBase<MutexType, CondVarType>::getZoneWriter(
                   << "/" << rrclass << ": not found in any configured "
                   "data source.");
     case datasrc::ConfigurableClientList::ZONE_NOT_CACHED:
-        isc_throw(InternalCommandError, "failed to load zone " << origin
-                  << "/" << rrclass << ": not served from memory");
+        LOG_DEBUG(auth_logger, DBG_AUTH_OPS,
+                  AUTH_DATASRC_CLIENTS_BUILDER_LOAD_ZONE_NOCACHE)
+            .arg(origin).arg(rrclass);
+        break;                  // return NULL below
     case datasrc::ConfigurableClientList::CACHE_DISABLED:
         // This is an internal error. Auth server must have the cache
         // enabled.
@@ -636,8 +647,6 @@ DataSrcClientsBuilderBase<MutexType, CondVarType>::getZoneWriter(
                   "is somehow disabled");
     }
 
-    // all cases above should either return or throw, but some compilers
-    // still need a return statement
     return (boost::shared_ptr<datasrc::memory::ZoneWriter>());
 }
 } // namespace datasrc_clientmgr_internal

src/bin/auth/tests/datasrc_clients_builder_unittest.cc

@@ -308,8 +308,12 @@ TEST_F(DataSrcClientsBuilderTest, loadZone) {
                                    "{\"class\": \"IN\","
                                    " \"origin\": \"test1.example\"}"));
     EXPECT_TRUE(builder.handleCommand(loadzone_cmd));
-    EXPECT_EQ(1, map_mutex.lock_count); // we should have acquired the lock
-    EXPECT_EQ(1, map_mutex.unlock_count); // and released it.
+
+    // loadZone involves two critical sections: one for getting the zone
+    // writer, and one for actually updating the zone data.  So the lock/unlock
+    // count should be incremented by 2.
+    EXPECT_EQ(2, map_mutex.lock_count);
+    EXPECT_EQ(2, map_mutex.unlock_count);
 
     newZoneChecks(clients_map, rrclass);
 }
@@ -381,7 +385,10 @@ TEST_F(DataSrcClientsBuilderTest,
               find(Name("example.org")).finder_->
               find(Name("example.org"), RRType::SOA())->code);
 
-    // attempt of reloading a zone but in-memory cache is disabled.
+    // attempt of reloading a zone but in-memory cache is disabled.  In this
+    // case the command is simply ignored.
+    const size_t orig_lock_count = map_mutex.lock_count;
+    const size_t orig_unlock_count = map_mutex.unlock_count;
     const ConstElementPtr config2(Element::fromJSON("{"
         "\"IN\": [{"
         "    \"type\": \"sqlite3\","
@@ -390,11 +397,13 @@ TEST_F(DataSrcClientsBuilderTest,
         "    \"cache-zones\": [\"example.org\"]"
         "}]}"));
     clients_map = configureDataSource(config2);
-    EXPECT_THROW(builder.handleCommand(
+    builder.handleCommand(
                      Command(LOADZONE, Element::fromJSON(
                                  "{\"class\": \"IN\","
-                                 " \"origin\": \"example.org\"}"))),
-                 TestDataSrcClientsBuilder::InternalCommandError);
+                                 " \"origin\": \"example.org\"}")));
+    // Only one mutex was needed because there was no actual reload.
+    EXPECT_EQ(orig_lock_count + 1, map_mutex.lock_count);
+    EXPECT_EQ(orig_unlock_count + 1, map_mutex.unlock_count);
 
     // basically impossible case: in-memory cache is completely disabled.
     // In this implementation of manager-builder, this should never happen,