[3407] Addressed review comments

Added missing parameter and method commentary.
Extended unit tests as suggested.
Moved onreceipt_handler_ from SignalSet to
anonymous namespace.

src/bin/d2/d_controller.h

@@ -405,6 +405,9 @@ protected:
     /// @return returns an Element that contains the results of shutdown
     /// command composed of an integer status value (0 means successful,
     /// non-zero means failure), and a string explanation of the outcome.
+    ///
+    /// @param args is a set of derivation-specific arguments (if any)
+    /// for the shutdown command.
     isc::data::ConstElementPtr shutdownProcess(isc::data::ConstElementPtr args);
 
     /// @brief Initializes signal handling
@@ -437,6 +440,8 @@ protected:
     /// obtained from the IOSignal.  This allows derivations to supply a
     /// custom signal processing method, while ensuring IOSignalQueue
     /// integrity.
+    ///
+    /// @param sequence_id id of the IOSignal instance "received"
     void ioSignalHandler(IOSignalId sequence_id);
 
     /// @brief Fetches the current process

src/bin/d2/io_service_signal.h

@@ -108,6 +108,7 @@ public:
     IOSignal(asiolink::IOService& io_service, int signum,
               IOSignalHandler handler);
 
+    /// @brief Destructor
     ~IOSignal();
 
     /// @brief Static method for generating IOSignal sequence_ids.
@@ -203,7 +204,7 @@ public:
     /// @throw IOSignalError if io_service is NULL.
     IOSignalQueue (IOServicePtr& io_service);
 
-    /// Destructor.
+    /// @brief Destructor.
     ~IOSignalQueue();
 
     /// @brief Creates an IOSignal

src/bin/d2/tests/d_controller_unittests.cc

@@ -43,7 +43,7 @@ public:
 
     virtual ~DStubControllerTest() {
     }
-    
+
     DStubControllerPtr controller_;
 };
 
@@ -355,7 +355,7 @@ TEST_F(DStubControllerTest, ioSignals) {
     // base class signal handler.
     controller_->recordSignalOnly(true);
 
-    // Setup to raise SIGHUP in 10 ms. 
+    // Setup to raise SIGHUP in 10 ms.
     TimedSignal sighup(*getIOService(), SIGHUP, 10);
     TimedSignal sigint(*getIOService(), SIGINT, 10);
     TimedSignal sigterm(*getIOService(), SIGTERM, 10);
@@ -380,17 +380,17 @@ TEST_F(DStubControllerTest, invalidConfigReload) {
     // new content is invalid JSON which will cause the config parse to fail.
     scheduleTimedWrite("{ \"string_test\": BOGUS JSON }", 100);
 
-    // Setup to raise SIGHUP in 200 ms. 
+    // Setup to raise SIGHUP in 200 ms.
     TimedSignal sighup(*getIOService(), SIGHUP, 200);
 
     // Write the config and then run launch() for 500 ms
-    // After startup, which will load the initial configuration this enters 
-    // the process's runIO() loop. We will first rewrite the config file. 
+    // After startup, which will load the initial configuration this enters
+    // the process's runIO() loop. We will first rewrite the config file.
     // Next we process the SIGHUP signal which should cause us to reconfigure.
     time_duration elapsed_time;
     runWithConfig("{ \"string_test\": \"first value\" }", 500, elapsed_time);
 
-    // Context is still available post launch. Check to see that our 
+    // Context is still available post launch. Check to see that our
     // configuration value is still the original value.
     std::string  actual_value = "";
     ASSERT_NO_THROW(getContext()->getParam("string_test", actual_value));
@@ -409,28 +409,30 @@ TEST_F(DStubControllerTest, validConfigReload) {
     // file is updated after we have done the initial configuration.
     scheduleTimedWrite("{ \"string_test\": \"second value\" }", 100);
 
-    // Setup to raise SIGHUP in 200 ms. 
+    // Setup to raise SIGHUP in 200 ms and another at 400 ms.
     TimedSignal sighup(*getIOService(), SIGHUP, 200);
+    TimedSignal sighup2(*getIOService(), SIGHUP, 400);
 
-    // Write the config and then run launch() for 500 ms
+    // Write the config and then run launch() for 800 ms
     time_duration elapsed_time;
-    runWithConfig("{ \"string_test\": \"first value\" }", 500, elapsed_time);
+    runWithConfig("{ \"string_test\": \"first value\" }", 800, elapsed_time);
 
-    // Context is still available post launch. 
+    // Context is still available post launch.
     // Check to see that our configuration value is what we expect.
     std::string  actual_value = "";
     ASSERT_NO_THROW(getContext()->getParam("string_test", actual_value));
     EXPECT_EQ("second value", actual_value);
 
-    // Verify that we saw the signal.
+    // Verify that we saw two occurrences of the signal.
     std::vector<int>& signals = controller_->getProcessedSignals();
-    ASSERT_EQ(1, signals.size());
+    ASSERT_EQ(2, signals.size());
     EXPECT_EQ(SIGHUP, signals[0]);
+    EXPECT_EQ(SIGHUP, signals[1]);
 }
 
 // Tests that the SIGINT triggers a normal shutdown.
 TEST_F(DStubControllerTest, sigintShutdown) {
-    // Setup to raise SIGHUP in 1 ms. 
+    // Setup to raise SIGHUP in 1 ms.
     TimedSignal sighup(*getIOService(), SIGINT, 1);
 
     // Write the config and then run launch() for 1000 ms
@@ -448,7 +450,7 @@ TEST_F(DStubControllerTest, sigintShutdown) {
 
 // Tests that the SIGTERM triggers a normal shutdown.
 TEST_F(DStubControllerTest, sigtermShutdown) {
-    // Setup to raise SIGHUP in 1 ms. 
+    // Setup to raise SIGHUP in 1 ms.
     TimedSignal sighup(*getIOService(), SIGTERM, 1);
 
     // Write the config and then run launch() for 1000 ms

src/bin/d2/tests/d_test_stubs.h

@@ -230,6 +230,8 @@ public:
     /// DControllerBase::processSignals will also be invoked. This switch is
     /// useful for ensuring that IOSignals are delivered as expected without
     /// incurring the full impact such as reconfiguring or shutting down.
+    ///
+    /// @param value boolean which if true enables record-only behavior
     void recordSignalOnly(bool value) {
        record_signal_only_ = value;
     }
@@ -280,6 +282,14 @@ protected:
     /// @return returns a string containing the option letters.
     virtual const std::string getCustomOpts() const;
 
+    /// @brief Application-level "signal handler"
+    ///
+    /// Overrides the base class implementation such that this method
+    /// is invoked each time an IOSignal is processed.  It records the
+    /// signal received and unless we are in record-only behavior, it
+    /// in invokes the base class implementation.
+    ///
+    /// @param signum OS signal value received
     virtual void processSignal(int signum);
 
 private:

src/bin/d2/tests/io_service_signal_unittests.cc

@@ -139,6 +139,7 @@ void dummyHandler(IOSignalId) {
 TEST(IOSignal, construction) {
     IOServicePtr io_service(new asiolink::IOService());
     IOSignalPtr signal;
+    IOSignalPtr signal2;
 
     // Verify that handler cannot be empty.
     ASSERT_THROW(signal.reset(new IOSignal(*io_service, SIGINT,
@@ -153,6 +154,16 @@ TEST(IOSignal, construction) {
 
     // Verify SIGINT is correct.
     EXPECT_EQ(SIGINT, signal->getSignum());
+
+    // Make a second signal.
+    ASSERT_NO_THROW(signal2.reset(new IOSignal(*io_service, SIGUSR1,
+                                               dummyHandler)));
+
+    // Verify sequence_id is not the same as the previous one.
+    EXPECT_NE(signal2->getSequenceId(), signal->getSequenceId());
+
+    // Verify that the signal value is correct.
+    EXPECT_EQ(SIGUSR1, signal2->getSignum());
 }
 
 // Tests IOSignalQueue constructors and exercises queuing methods.
@@ -168,22 +179,52 @@ TEST(IOSignalQueue, constructionAndQueuing) {
     ASSERT_NO_THROW(queue.reset(new IOSignalQueue(io_service)));
 
     // Verify an empty handler is not allowed.
-    ASSERT_THROW(queue->pushSignal(SIGINT, IOSignalHandler()),
-                 IOSignalError);
+    ASSERT_THROW(queue->pushSignal(SIGINT, IOSignalHandler()), IOSignalError);
+
+    // Verify that we can queue valid entries.
+    std::vector<IOSignalId> ids;
+    ASSERT_NO_THROW(ids.push_back(queue->pushSignal(SIGINT, dummyHandler)));
+    ASSERT_NO_THROW(ids.push_back(queue->pushSignal(SIGUSR1, dummyHandler)));
+    ASSERT_NO_THROW(ids.push_back(queue->pushSignal(SIGUSR2, dummyHandler)));
 
-    // Verify we can queue up a valid entry.
-    IOSignalId sequence_id = queue->pushSignal(SIGINT, dummyHandler);
+    // Now verify that we can pop each one and what we pop is correct.
+    // Verify popping it again, throws.  We'll do it in a non-sequential order.
 
-    // Verify we can pop the entry.
-    IOSignalPtr signal = queue->popSignal(sequence_id);
+    // Check the middle one.
+    IOSignalPtr signal;
+    ASSERT_NO_THROW(signal = queue->popSignal(ids[1]));
     ASSERT_TRUE(signal);
+    EXPECT_EQ(ids[1], signal->getSequenceId());
+    EXPECT_EQ(SIGUSR1, signal->getSignum());
+    ASSERT_THROW(queue->popSignal(ids[1]), IOSignalError);
 
-    // Verify the one we popped is right.
-    EXPECT_EQ(sequence_id, signal->getSequenceId());
+    // Check the first one.
+    ASSERT_NO_THROW(signal = queue->popSignal(ids[0]));
+    ASSERT_TRUE(signal);
+    EXPECT_EQ(ids[0], signal->getSequenceId());
     EXPECT_EQ(SIGINT, signal->getSignum());
+    ASSERT_THROW(queue->popSignal(ids[0]), IOSignalError);
+
+    // Check the last one.
+    ASSERT_NO_THROW(signal = queue->popSignal(ids[2]));
+    ASSERT_TRUE(signal);
+    EXPECT_EQ(ids[2], signal->getSequenceId());
+    EXPECT_EQ(SIGUSR2, signal->getSignum());
+    ASSERT_THROW(queue->popSignal(ids[2]), IOSignalError);
+
+    // Now we will test clearing the queue.  Queue three signals.
+    ids.clear();
+    for (int i = 0; i < 3; ++i) {
+        ASSERT_NO_THROW(ids.push_back(queue->pushSignal(SIGINT, dummyHandler)));
+    }
+
+    // Now clear the queue.
+    ASSERT_NO_THROW(queue->clear());
 
-    // Verify popping it again, throws.
-    ASSERT_THROW(queue->popSignal(sequence_id), IOSignalError);
+    // We should not be able to dequeue any of them.
+    for (int i = 0; i < 3; ++i) {
+        ASSERT_THROW(queue->popSignal(ids[i]), IOSignalError);
+    }
 }
 
 // Test the basic mechanics of IOSignal by handling one signal occurrence.

src/lib/util/signal_set.cc

@@ -78,17 +78,17 @@ void internalHandler(int sig) {
     states->push_back(sig);
 }
 
+/// @brief Optional handler to execute at the time of signal receipt
+BoolSignalHandler onreceipt_handler_ = BoolSignalHandler();
+
 }; // end anon namespace
 
 namespace isc {
 namespace util {
 
-const BoolSignalHandler SignalSet::EMPTY_BOOL_HANDLER = BoolSignalHandler();
-BoolSignalHandler SignalSet::onreceipt_handler_ = EMPTY_BOOL_HANDLER;
-
 bool
 SignalSet::invokeOnReceiptHandler(int sig) {
-    if (!SignalSet::onreceipt_handler_) {
+    if (!onreceipt_handler_) {
         return (false);
     }
 
@@ -109,7 +109,7 @@ SignalSet::invokeOnReceiptHandler(int sig) {
     // Call the registered handler.
     bool signal_processed = false;
     try {
-        signal_processed = SignalSet::onreceipt_handler_(sig);
+        signal_processed = onreceipt_handler_(sig);
     } catch (const std::exception& ex) {
         // Restore the handler.  We might fail to restore it, but we likely
         // have bigger issues anyway.
@@ -291,7 +291,7 @@ SignalSet::setOnReceiptHandler(BoolSignalHandler handler) {
 
 void
 SignalSet::clearOnReceiptHandler() {
-    onreceipt_handler_ = EMPTY_BOOL_HANDLER;
+    onreceipt_handler_ = BoolSignalHandler();
 }
 
 } // end of isc::util

src/lib/util/signal_set.h

@@ -79,10 +79,6 @@ typedef boost::function<bool(int signum)> BoolSignalHandler;
 /// signals' queue.
 class SignalSet : public boost::noncopyable {
 public:
-    /// @brief Optional handler to execute at the time of signal receipt
-    static BoolSignalHandler onreceipt_handler_;
-    static const BoolSignalHandler EMPTY_BOOL_HANDLER;
-
     /// @brief Constructor installing one signal.
     ///
     /// @param sig0 First signal.