[1068] one missing point: have the DatabaseUpdater's destructor catch

catch exception in rollback explicitly.  added a test case for that.

src/lib/datasrc/database.cc

@@ -627,9 +627,20 @@ public:
 
     virtual ~DatabaseUpdater() {
         if (!committed_) {
-            accessor_->rollbackUpdateZone();
-            logger.info(DATASRC_DATABASE_UPDATER_ROLLBACK)
-                .arg(zone_name_).arg(zone_class_).arg(db_name_);
+            try {
+                accessor_->rollbackUpdateZone();
+                logger.info(DATASRC_DATABASE_UPDATER_ROLLBACK)
+                    .arg(zone_name_).arg(zone_class_).arg(db_name_);
+            } catch (const DataSourceError& e) {
+                // We generally expect that rollback always succeeds, and
+                // it should in fact succeed in a way we execute it.  But
+                // as the public API allows rollbackUpdateZone() to fail and
+                // throw, we should expect it.  Obviously we cannot re-throw
+                // it.  The best we can do is to log it as a critical error.
+                logger.error(DATASRC_DATABASE_UPDATER_ROLLBACKFAIL)
+                    .arg(zone_name_).arg(zone_class_).arg(db_name_)
+                    .arg(e.what());
+            }
         }
 
         logger.debug(DBG_TRACE_DATA, DATASRC_DATABASE_UPDATER_DESTROYED)

src/lib/datasrc/datasrc_messages.mes

@@ -608,6 +608,19 @@ the changes.  The intermediate changes made through the updater won't
 be applied to the underlying database.  The zone name, its class, and
 the underlying database name are shown in the log message.
 
+%DATASRC_DATABASE_UPDATER_ROLLBACKFAIL failed to roll back zone updates for '%1/%2' on %3: %4
+A zone updater is being destroyed without committing the changes to
+the database, and attempts to rollback incomplete updates, but it
+unexpectedly fails.  The higher level implementation does not expect
+it to fail, so this means either a serious operational error in the
+underlying data source (such as a system failure of a database) or
+software bug in the underlying data source implementation.  In either
+case if this message is logged the administrator should carefully
+examine the underlying data source to see what exactly happens and
+whether the data is still valid.  The zone name, its class, and the
+underlying database name as well as the error message thrown from the
+database module are shown in the log message.
+
 % DATASRC_DATABASE_UPDATER_COMMIT updates committed for '%1/%2' on %3
 Debug information.  A set of updates to a zone has been successfully
 committed to the corresponding database backend.  The zone name,

src/lib/datasrc/sqlite3_accessor.cc

@@ -111,8 +111,7 @@ public:
     }
 
     void exec() {
-        if (sqlite3_step(dbparameters_.statements_[stmt_id_]) !=
-            SQLITE_DONE) {
+        if (sqlite3_step(dbparameters_.statements_[stmt_id_]) != SQLITE_DONE) {
             sqlite3_reset(dbparameters_.statements_[stmt_id_]);
             isc_throw(DataSourceError, "failed to " << desc_ << ": " <<
                       sqlite3_errmsg(dbparameters_.db_));

src/lib/datasrc/tests/database_unittest.cc

@@ -328,6 +328,13 @@ public:
         *readonly_records_ = *update_records_;
     }
     virtual void rollbackUpdateZone() {
+        // Special hook: if something with a name of "throw.example.org"
+        // has been added, trigger an imaginary unexpected event with an
+        // exception.
+        if (update_records_->count("throw.example.org.") > 0) {
+            isc_throw(DataSourceError, "unexpected failure in rollback");
+        }
+
         rollbacked_ = true;
     }
     virtual void addRecordToZone(const string (&columns)[ADD_COLUMN_COUNT]) {
@@ -1572,6 +1579,19 @@ TEST_F(DatabaseClientTest, updateCancel) {
     EXPECT_EQ(ZoneFinder::SUCCESS, finder->find(qname_, qtype_).code);
 }
 
+TEST_F(DatabaseClientTest, exceptionFromRollback) {
+    updater_ = client_->getUpdater(zname_, true);
+
+    rrset_.reset(new RRset(Name("throw.example.org"), qclass_, qtype_,
+                           rrttl_));
+    rrset_->addRdata(rdata::createRdata(rrset_->getType(),
+                                        rrset_->getClass(), "192.0.2.1"));
+    updater_->addRRset(*rrset_);
+    // destruct without commit.  The added name will result in an exception
+    // in the MockAccessor's rollback method.  It shouldn't be propagated.
+    EXPECT_NO_THROW(updater_.reset());
+}
+
 TEST_F(DatabaseClientTest, duplicateCommit) {
     // duplicate commit.  should result in exception.
     updater_ = client_->getUpdater(zname_, true);