[640] address number of review comments

src/lib/config/ccsession.h

@@ -192,12 +192,12 @@ public:
                     bool handle_logging = true
                     );
 
-    /**
-     * Destructor
-     *
-     * The desctructor automatically calls sendStopping(), which sends
-     * a message to the ConfigManager that this module is stopping
-     */
+    ///
+    /// Destructor
+    ///
+    /// The desctructor automatically calls sendStopping(), which sends
+    /// a message to the ConfigManager that this module is stopping
+    ///
     ~ModuleCCSession();
 
     /// Start receiving new commands and configuration changes asynchronously.

src/lib/config/config_messages.mes

@@ -49,6 +49,11 @@ manager is appended to the log error. The most likely cause is that
 the module is of a different (command specification) version than the
 running configuration manager.
 
+% CONFIG_JSON_PARSE JSON parse error in %1: %2
+There was an error parsing the JSON file. The given file does not appear
+to be in valid JSON format. Please verify that the filename is correct
+and that the contents are valid JSON.
+
 % CONFIG_LOG_EXPLICIT will use logging configuration for explicitly-named logger %1
 This is a debug message.  When processing the "loggers" part of the
 configuration file, the configuration library found an entry for the named
@@ -74,11 +79,6 @@ wildcard entry (one containing the "*" character) that matches a logger
 specification in the program. The logging configuration for the program
 will be updated with the information.
 
-% CONFIG_JSON_PARSE JSON parse error in %1: %2
-There was an error parsing the JSON file. The given file does not appear
-to be in valid JSON format. Please verify that the filename is correct
-and that the contents are valid JSON.
-
 % CONFIG_MOD_SPEC_FORMAT module specification error in %1: %2
 The given file does not appear to be a valid specification file: details
 are included in the message. Please verify that the filename is correct

src/lib/config/tests/ccsession_unittests.cc

@@ -26,6 +26,8 @@
 
 #include <log/logger_name.h>
 
+#include <boost/scoped_ptr.hpp>
+
 using namespace isc::data;
 using namespace isc::config;
 using namespace isc::cc;
@@ -197,17 +199,20 @@ TEST_F(CCSessionTest, session_close) {
     std::string group, to;
 
     EXPECT_FALSE(session.haveSubscription("Spec2", "*"));
-    {
-        ModuleCCSession mccs(ccspecfile("spec2.spec"), session, NULL, NULL,
-                             true, false);
-        EXPECT_TRUE(session.haveSubscription("Spec2", "*"));
-        // The initial message is irrelevant for this test
-        // (see session2 test), drop it
-        session.getFirstMessage(group, to);
-        // Queue should now be empty
-        ASSERT_EQ(0, session.getMsgQueue()->size());
-    }
-    // Destructor should have cause a new message
+
+    boost::scoped_ptr<ModuleCCSession> mccs(new ModuleCCSession(
+                                         ccspecfile("spec2.spec"),
+                                         session, NULL, NULL,
+                                         true, false));
+    EXPECT_TRUE(session.haveSubscription("Spec2", "*"));
+    // The initial message is irrelevant for this test
+    // (see session2 test), drop it
+    session.getFirstMessage(group, to);
+    // Queue should now be empty
+    ASSERT_EQ(0, session.getMsgQueue()->size());
+    // Invoke the destructor
+    mccs.reset();
+    // Destructor should have caused a new message
     ASSERT_EQ(1, session.getMsgQueue()->size());
     msg = session.getFirstMessage(group, to);
     EXPECT_EQ("{ \"command\": [ \"stopping\", "
@@ -217,6 +222,37 @@ TEST_F(CCSessionTest, session_close) {
     EXPECT_EQ(0, session.getMsgQueue()->size());
 }
 
+TEST_F(CCSessionTest, session_close_exception) {
+    // Test whether an exception encountered during the destructor is
+    // handled correctly
+    ConstElementPtr msg;
+    std::string group, to;
+
+    EXPECT_FALSE(session.haveSubscription("Spec2", "*"));
+
+    boost::scoped_ptr<ModuleCCSession> mccs(new ModuleCCSession(
+                                         ccspecfile("spec2.spec"),
+                                         session, NULL, NULL,
+                                         true, false));
+    EXPECT_TRUE(session.haveSubscription("Spec2", "*"));
+    // The initial message is irrelevant for this test
+    // (see session2 test), drop it
+    session.getFirstMessage(group, to);
+    // Queue should now be empty
+    ASSERT_EQ(0, session.getMsgQueue()->size());
+
+    // Set fake session to throw an exception
+    session.setThrowOnSend(true);
+
+    // Invoke the destructor
+    mccs.reset();
+    // Destructor should not have caused a new message (since fakesession
+    // should have thrown an exception)
+    ASSERT_EQ(0, session.getMsgQueue()->size());
+    //EXPECT_EQ(0, session.getMsgQueue()->size());
+}
+
+
 ConstElementPtr my_config_handler(ConstElementPtr new_config) {
     if (new_config && new_config->contains("item1") &&
         new_config->get("item1")->intValue() == 5) {

src/lib/config/tests/fake_session.cc

@@ -72,7 +72,8 @@ FakeSession::FakeSession(isc::data::ElementPtr initial_messages,
     messages_(initial_messages),
     subscriptions_(subscriptions),
     msg_queue_(msg_queue),
-    started_(false)
+    started_(false),
+    throw_on_send_(false)
 {
 }
 
@@ -181,8 +182,9 @@ int
 FakeSession::group_sendmsg(ConstElementPtr msg, std::string group,
                            std::string to, std::string)
 {
-    //cout << "[XX] client sends message: " << msg << endl;
-    //cout << "[XX] to: " << group << " . " << instance << "." << to << endl;
+    if (throw_on_send_) {
+        isc_throw(Exception, "Throw on send is set in FakeSession");
+    }
     addMessage(msg, group, to);
     return (1);
 }
@@ -261,6 +263,5 @@ FakeSession::haveSubscription(ConstElementPtr group, ConstElementPtr instance)
 {
     return (haveSubscription(group->stringValue(), instance->stringValue()));
 }
-
 }
 }

src/lib/config/tests/fake_session.h

@@ -87,6 +87,14 @@ public:
     isc::data::ElementPtr getMessages() { return (messages_); }
     isc::data::ElementPtr getMsgQueue() { return (msg_queue_); }
 
+    /// Throw exception on sendmsg()
+    ///
+    /// When set to true, and sendmsg() is later called, this
+    /// will throw isc::Exception
+    ///
+    /// \param value If true, enable throw. If false, disable it
+    void setThrowOnSend(bool value) { throw_on_send_ = value; }
+
 private:
     bool recvmsg(isc::data::ConstElementPtr& msg,
                  bool nonblock = true, int seq = -1);
@@ -98,6 +106,7 @@ private:
     isc::data::ElementPtr subscriptions_;
     isc::data::ElementPtr msg_queue_;
     bool started_;
+    bool throw_on_send_;
 };
 } // namespace cc
 } // namespace isc

src/lib/python/isc/config/ccsession.py

@@ -216,15 +216,18 @@ class ModuleCCSession(ConfigData):
            message is just an FYI, and no response is expected. Any errors
            when sending this message (for instance if the msgq session has
            previously been closed) are logged, but ignored."""
+        # create_command could raise an exception as well, but except for
+        # out of memory related errors, these should all be programming
+        # failures and are not caught
         msg = create_command(COMMAND_MODULE_STOPPING,
                              self.get_module_spec().get_full_spec())
         try:
             self._session.group_sendmsg(msg, "ConfigManager")
-        except isc.cc.session.SessionError as se:
+        except Exception as se:
             # If the session was previously closed, obvously trying to send
             # a message fails. (TODO: check if session is open so we can
             # error on real problems?)
-            logger.error(CONFIG_SESSION_STOPPING_FAILED, str(se))
+            logger.error(CONFIG_SESSION_STOPPING_FAILED, se)
 
     def get_socket(self):
         """Returns the socket from the command channel session. This

src/lib/python/isc/config/cfgmgr.py

@@ -531,4 +531,6 @@ class ConfigManager:
             # not ask
             if msg is not None and not 'result' in msg:
                 answer = self.handle_msg(msg);
-                self.cc.group_reply(env, answer)
+                # Only respond if there actually is something to respond with
+                if answer is not None:
+                    self.cc.group_reply(env, answer)

src/lib/python/isc/config/tests/cfgmgr_test.py

@@ -495,9 +495,14 @@ class TestConfigManager(unittest.TestCase):
     def test_run(self):
         self.fake_session.group_sendmsg({ "command": [ "get_commands_spec" ] }, "ConfigManager")
         self.fake_session.group_sendmsg({ "command": [ "get_statistics_spec" ] }, "ConfigManager")
+        self.fake_session.group_sendmsg({ "command": [ "stopping", { "module_name": "FooModule" } ] }, "ConfigManager")
         self.fake_session.group_sendmsg({ "command": [ "shutdown" ] }, "ConfigManager")
+        self.assertEqual(len(self.fake_session.message_queue), 4)
         self.cm.run()
-        pass
+        # All commands should have been read out by run()
+        # Three of the commands should have been responded to, so the queue
+        # should now contain three answers
+        self.assertEqual(len(self.fake_session.message_queue), 3)
 
 
 if __name__ == '__main__':