addressed the small issues from jinmei's review (i.e. everything except the...

addressed the small issues from jinmei's review (i.e. everything except the first two points and the one about integer sizes)


git-svn-id: svn://bind10.isc.org/svn/bind10/branches/trac296@2699 e5f2f494-b856-4b98-b285-d166d9295462

src/bin/auth/tests/auth_srv_unittest.cc

@@ -95,7 +95,7 @@ private:
         virtual int reply(ElementPtr& envelope, ElementPtr& newmsg);
         virtual bool hasQueuedMsgs();
         virtual void setTimeout(size_t timeout UNUSED_PARAM) {};
-        virtual size_t getTimeout() { return 0; };
+        virtual size_t getTimeout() const { return 0; };
 
         void setMessage(ElementPtr msg) { msg_ = msg; }
         void disableSend() { send_ok_ = false; }

src/lib/cc/session.cc

@@ -56,6 +56,16 @@ using namespace isc::data;
 // (e.g. write(2)) so we don't import the entire asio namespace.
 using asio::io_service;
 
+// By default, unless changed or disabled, blocking reads on
+// the msgq channel will time out after 4 seconds in this
+// implementation.
+// This number is chosen to be low enough so that whatever
+// component is blocking does not seem to be hanging, but
+// still gives enough time for other modules to respond if they
+// are busy. If this choice turns out to be a bad one, we can
+// change it later.
+#define BIND10_MSGQ_DEFAULT_TIMEOUT 4000
+
 namespace isc {
 namespace cc {
 class SessionImpl {
@@ -63,7 +73,7 @@ public:
     SessionImpl(io_service& io_service) :
         sequence_(-1), queue_(Element::createList()),
         io_service_(io_service), socket_(io_service_), data_length_(0),
-        timeout_(4000)
+        timeout_(BIND10_MSGQ_DEFAULT_TIMEOUT)
     {}
     void establish(const char& socket_file);
     void disconnect();
@@ -73,8 +83,8 @@ public:
     // (in seconds) is thrown. If timeout is 0 it will block forever
     void readData(void* data, size_t datalen);
     void startRead(boost::function<void()> user_handler);
-    virtual void setTimeout(size_t seconds) { timeout_ = seconds; };
-    virtual size_t getTimeout() { return timeout_; };
+    void setTimeout(size_t seconds) { timeout_ = seconds; };
+    size_t getTimeout() const { return timeout_; };
 
     long int sequence_; // the next sequence number to use
     std::string lname_;
@@ -474,7 +484,7 @@ Session::setTimeout(size_t milliseconds) {
 }
 
 size_t
-Session::getTimeout() {
+Session::getTimeout() const {
     return (impl_->getTimeout());
 }
 }

src/lib/cc/session.h

@@ -97,8 +97,17 @@ namespace isc {
             virtual int reply(isc::data::ElementPtr& envelope,
                                isc::data::ElementPtr& newmsg) = 0;
             virtual bool hasQueuedMsgs() = 0;
+
+            /// \brief Sets the default timeout for blocking reads
+            ///        in this session to the given number of milliseconds
+            /// \param milliseconds the timeout for blocking reads in
+            ///        milliseconds, if this is set to 0, reads will block
+            ///        forever.
             virtual void setTimeout(size_t milliseconds) = 0;
-            virtual size_t getTimeout() = 0;
+
+            /// \brief Returns the current timeout for blocking reads
+            /// \return The timeout (in milliseconds)
+            virtual size_t getTimeout() const = 0;
         };
 
     class Session : public AbstractSession {
@@ -132,15 +141,8 @@ namespace isc {
             virtual int reply(isc::data::ElementPtr& envelope,
                               isc::data::ElementPtr& newmsg);
             virtual bool hasQueuedMsgs();
-            /// \brief Set the default timeout for blocking reads
-            ///        in this session to the given number of milliseconds
-            /// \param milliseconds the timeout for blocking reads in
-            ///        milliseconds, if this is set to 0, reads will block
-            ///        forever. Defaults to 4000.
             virtual void setTimeout(size_t milliseconds);
-            /// \brief Returns the current timeout for blocking reads
-            /// \return The timeout (in milliseconds)
-            virtual size_t getTimeout();
+            virtual size_t getTimeout() const;
     private:
             void sendmsg(isc::data::ElementPtr& msg);
             void sendmsg(isc::data::ElementPtr& env,

src/lib/cc/session_unittests.cc

@@ -120,12 +120,26 @@ private:
     char data_buf[1024];
 };
 
+class SessionTest : public ::testing::Test {
+protected:
+    SessionTest() {
+        // The TestDomainSocket is held as a 'new'-ed pointer,
+        // so we can call unlink() first.
+        unlink(BIND10_TEST_SOCKET_FILE);
+        tds = new TestDomainSocket(my_io_service, BIND10_TEST_SOCKET_FILE);
+    }
+
+    ~SessionTest() {
+        delete tds;
+    }
 
-TEST(Session, timeout_on_connect) {
     asio::io_service my_io_service;
-    ::unlink(BIND10_TEST_SOCKET_FILE);
-    TestDomainSocket tds(my_io_service, BIND10_TEST_SOCKET_FILE);
+    TestDomainSocket* tds;
+};
+
+TEST_F(SessionTest, timeout_on_connect) {
     Session sess(my_io_service);
+    
     // set to a short timeout so the test doesn't take too long
     EXPECT_EQ(4000, sess.getTimeout());
     sess.setTimeout(100);
@@ -134,31 +148,21 @@ TEST(Session, timeout_on_connect) {
     EXPECT_THROW(sess.establish(BIND10_TEST_SOCKET_FILE), SessionTimeout);
 }
 
-TEST(Session, connect_ok) {
-    asio::io_service my_io_service;
-    ::unlink(BIND10_TEST_SOCKET_FILE);
-    TestDomainSocket tds(my_io_service, BIND10_TEST_SOCKET_FILE);
-    tds.setSendLname();
+TEST_F(SessionTest, connect_ok) {
+    tds->setSendLname();
 
     Session sess(my_io_service);
     sess.establish(BIND10_TEST_SOCKET_FILE);
 }
 
-TEST(Session, connect_ok_connection_reset) {
-    asio::io_service my_io_service;
-    ::unlink(BIND10_TEST_SOCKET_FILE);
+TEST_F(SessionTest, connect_ok_connection_reset) {
+    tds->setSendLname();
+
     Session sess(my_io_service);
+    sess.establish(BIND10_TEST_SOCKET_FILE);
+    // Close the session again, so the next recv() should throw
+    sess.disconnect();
 
-    // Create a fake msgq in a smaller scope, so we can
-    // connect the session to it, but later calls on the
-    // underlying socket will fail
-    {
-        TestDomainSocket tds(my_io_service, BIND10_TEST_SOCKET_FILE);
-        tds.setSendLname();
-        sess.establish(BIND10_TEST_SOCKET_FILE);
-    }
-    
     isc::data::ElementPtr env, msg;
     EXPECT_THROW(sess.group_recvmsg(env, msg, false, -1), SessionError);
 }
-

src/lib/config/tests/fake_session.h

@@ -73,7 +73,7 @@ public:
                       isc::data::ElementPtr& newmsg);
     virtual bool hasQueuedMsgs();
     virtual void setTimeout(size_t milliseconds) {};
-    virtual size_t getTimeout() { return 0; };
+    virtual size_t getTimeout() const { return 0; };
     isc::data::ElementPtr getFirstMessage(std::string& group, std::string& to);
     void addMessage(isc::data::ElementPtr, const std::string& group,
                     const std::string& to);