[trac931] Different test for the double read

src/lib/cc/tests/session_unittests.cc

@@ -235,4 +235,9 @@ TEST_F(SessionTest, run_with_handler_timeout) {
     ASSERT_THROW(my_io_service.run(), SessionTimeout);
 }
 
+// Test it throws if we start it twice
+TEST_F(SessionTest, doubleStart) {
+    sess.start();
+}
+
 

src/lib/config/ccsession.cc

@@ -252,6 +252,8 @@ ModuleCCSession::start() {
 
     // register callback for asynchronous read
     session_.startRead(boost::bind(&ModuleCCSession::startCheck, this));
+
+    started_ = true;
 }
 
 /// Validates the new config values, if they are correct,

src/lib/config/tests/ccsession_unittests.cc

@@ -362,7 +362,7 @@ TEST_F(CCSessionTest, remoteConfig) {
     std::string module_name;
     int item1;
     
-    ModuleCCSession mccs(ccspecfile("spec1.spec"), session, NULL, NULL);
+    ModuleCCSession mccs(ccspecfile("spec1.spec"), session, NULL, NULL, false);
     EXPECT_TRUE(session.haveSubscription("Spec1", "*"));
     
     // first simply connect, with no config values, and see we get
@@ -505,7 +505,8 @@ TEST_F(CCSessionTest, ignoreRemoteConfigCommands) {
     session.getMessages()->add(createAnswer(0, el("{  }")));
 
     EXPECT_FALSE(session.haveSubscription("Spec29", "*"));
-    ModuleCCSession mccs(ccspecfile("spec29.spec"), session, my_config_handler, my_command_handler);
+    ModuleCCSession mccs(ccspecfile("spec29.spec"), session, my_config_handler,
+                         my_command_handler, false);
     EXPECT_TRUE(session.haveSubscription("Spec29", "*"));
 
     EXPECT_EQ(2, session.getMsgQueue()->size());
@@ -555,4 +556,31 @@ TEST_F(CCSessionTest, initializationFail) {
     EXPECT_TRUE(session.haveSubscription("Spec29", "*"));
 }
 
+// Test it throws when we try to start it twice (once from the constructor)
+TEST_F(CCSessionTest, doubleStartImplicit) {
+    ModuleCCSession mccs(ccspecfile("spec29.spec"), session, NULL, NULL);
+    EXPECT_THROW(mccs.start(), CCSessionError);
+}
+
+// The same, but both starts are explicit
+TEST_F(CCSessionTest, doubleStartExplicit) {
+    ModuleCCSession mccs(ccspecfile("spec29.spec"), session, NULL, NULL,
+                         false);
+    mccs.start();
+    EXPECT_THROW(mccs.start(), CCSessionError);
+}
+
+// Test we can request synchronous receive before we start the session,
+// and check there's the mechanism if we do it after
+TEST_F(CCSessionTest, delayedStart) {
+    ModuleCCSession mccs(ccspecfile("spec2.spec"), session, NULL, NULL, false);
+    session.getMessages()->add(createAnswer());
+    ConstElementPtr env, answer;
+    EXPECT_NO_THROW(session.group_recvmsg(env, answer, false, 3));
+    mccs.start();
+    session.getMessages()->add(createAnswer());
+    EXPECT_THROW(session.group_recvmsg(env, answer, false, 3),
+                 FakeSession::DoubleRead);
+}
+
 }

src/lib/config/tests/fake_session.cc

@@ -71,7 +71,8 @@ FakeSession::FakeSession(isc::data::ElementPtr initial_messages,
                          isc::data::ElementPtr msg_queue) :
     messages_(initial_messages),
     subscriptions_(subscriptions),
-    msg_queue_(msg_queue)
+    msg_queue_(msg_queue),
+    started_(false)
 {
 }
 
@@ -84,6 +85,7 @@ FakeSession::disconnect() {
 
 void
 FakeSession::startRead(boost::function<void()>) {
+    started_ = true;
 }
 
 void
@@ -91,7 +93,13 @@ FakeSession::establish(const char*) {
 }
 
 bool
-FakeSession::recvmsg(ConstElementPtr& msg, bool, int) {
+FakeSession::recvmsg(ConstElementPtr& msg, bool nonblock, int) {
+    if (started_ && !nonblock) {
+        // This would schedule another read for length, leading to
+        // corputed data
+        isc_throw(DoubleRead, "Second read scheduled from recvmsg");
+    }
+
     //cout << "[XX] client asks for message " << endl;
     if (messages_ &&
         messages_->getType() == Element::list &&
@@ -105,7 +113,15 @@ FakeSession::recvmsg(ConstElementPtr& msg, bool, int) {
 }
 
 bool
-FakeSession::recvmsg(ConstElementPtr& env, ConstElementPtr& msg, bool, int) {
+FakeSession::recvmsg(ConstElementPtr& env, ConstElementPtr& msg, bool nonblock,
+                     int)
+{
+    if (started_ && !nonblock) {
+        // This would schedule another read for length, leading to
+        // corputed data
+        isc_throw(DoubleRead, "Second read scheduled from recvmsg");
+    }
+
     //cout << "[XX] client asks for message and env" << endl;
     env = ElementPtr();
     if (messages_ &&

src/lib/config/tests/fake_session.h

@@ -42,6 +42,14 @@ public:
                 isc::data::ElementPtr msg_queue);
     virtual ~FakeSession();
 
+    // This is thrown if two reads for length at once are scheduled at once.
+    // Such thing does bad things currently (see discussion in ticket #931).
+    class DoubleRead : public Exception {
+    public:
+        DoubleRead(const char* file, size_t line, const char* what) :
+            Exception(file, line, what) {}
+    };
+
     virtual void startRead(boost::function<void()> read_callback);
 
     virtual void establish(const char* socket_file = NULL);
@@ -89,6 +97,7 @@ private:
     const isc::data::ElementPtr messages_;
     isc::data::ElementPtr subscriptions_;
     isc::data::ElementPtr msg_queue_;
+    bool started_;
 };
 } // namespace cc
 } // namespace isc

src/lib/server_common/tests/keyring_test.cc

@@ -38,7 +38,7 @@ public:
         specfile(std::string(TEST_DATA_PATH) + "/spec.spec")
     {
         session.getMessages()->add(createAnswer());
-        mccs.reset(new ModuleCCSession(specfile, session, NULL, NULL));
+        mccs.reset(new ModuleCCSession(specfile, session, NULL, NULL, false));
     }
     isc::cc::FakeSession session;
     std::auto_ptr<ModuleCCSession> mccs;