libt2n: (tomj) fixed call of virtual function close() from destructor, fixed return...
authorThomas Jarosch <thomas.jarosch@intra2net.com>
Tue, 25 Nov 2008 18:45:41 +0000 (18:45 +0000)
committerThomas Jarosch <thomas.jarosch@intra2net.com>
Tue, 25 Nov 2008 18:45:41 +0000 (18:45 +0000)
17 files changed:
src/command_client.hxx
src/command_server.cpp
src/command_server.hxx
src/connection.cpp
src/server.cpp
src/server.hxx
src/socket_client.cpp
src/socket_client.hxx
src/socket_handler.cpp
src/socket_handler.hxx
src/socket_server.cpp
src/socket_server.hxx
test/newserver.cpp
test/reentrant.cpp
test/serialize.cpp
test/simplecmd.cpp
test/timeout.cpp

index 59d4755..49f2819 100644 (file)
@@ -45,6 +45,7 @@ class command_client
         std::string read_packet(const long long &usec_timeout);
         bool check_hello(const std::string& hellostr);
 
+        // TODO: Deny access to copy constructor or use boost::shared_ptr
         std::auto_ptr<t2n_exception> constructorException;
 
     public:
index e41d41b..8c4bb42 100644 (file)
@@ -50,6 +50,13 @@ command_server::command_server(server& _s)
     s.add_callback(new_connection,bind(&command_server::send_hello, boost::ref(*this), _1));
 }
 
+/**
+ * Destructor
+ */
+command_server::~command_server()
+{
+}
+
 /// send a hello message to a new connection
 void command_server::send_hello(unsigned int conn_id)
 {
@@ -166,19 +173,19 @@ void command_server::handle_packet(const std::string& packet, server_connection*
 void command_server::handle(long long usec_timeout, long long* usec_timeout_remaining)
 {
     guard_handle++;
+
     try
     {
         if (s.fill_buffer(usec_timeout,usec_timeout_remaining))
         {
             std::string packet;
-            unsigned int conn_id;
+            unsigned int conn_id = 0;
 
             while (s.get_packet(packet,conn_id))
             {
                 server_connection* conn=s.get_connection(conn_id);
                 if (!conn)
                     EXCEPTIONSTREAM(error,logic_error,"illegal connection id " << conn_id << " received");
-
                 try
                     { handle_packet(packet,conn); }
                 catch (t2n_transfer_error &e)
index 04508b5..97b3a07 100644 (file)
@@ -41,7 +41,7 @@ class command_server
 
     public:
         command_server(server& _s);
-        // TODO: No (virtual) destructor
+        ~command_server();
 
         void handle(long long usec_timeout=-1, long long* usec_timeout_remaining=NULL);
 
index cc543b2..4abfad4 100644 (file)
@@ -31,9 +31,13 @@ namespace libt2n
 
 connection::~connection()
 {
-    // we want the connection_closed callbacks to be called before
-    // FIXME: Functios is virtual
-    close();
+    // Run close() manually since it's a virtual function
+    // and we are in the destructor.
+    if (!is_closed())
+    {
+        closed=true;
+        do_callbacks(connection_closed);
+    }
 
     do_callbacks(connection_deleted);
 }
index 30bf7d3..749f4fb 100644 (file)
@@ -30,12 +30,19 @@ namespace libt2n
 {
 
 server_connection::server_connection(int _timeout)
-    : connection()
+    : connection_id(0)
+    , my_server(NULL)
+    , connection()
 {
     set_timeout(_timeout);
     reset_timeout();
-    connection_id=0;
-    my_server=0;
+}
+
+/**
+ * Destructor
+ */
+server_connection::~server_connection()
+{
 }
 
 /// get pointer to logging stream, returns NULL if no logging needed
@@ -94,6 +101,18 @@ server::~server()
     std::map<unsigned int, server_connection*>::iterator ie=connections.end();
     for(std::map<unsigned int, server_connection*>::iterator i=connections.begin(); i != ie; i++)
         delete i->second;
+
+    connections.clear();
+}
+
+/**
+ * Close all open connections
+ */
+void server::close()
+{
+    std::map<unsigned int, server_connection*>::iterator ie=connections.end();
+    for(std::map<unsigned int, server_connection*>::iterator i=connections.begin(); i != ie; ++i)
+        i->second->close();
 }
 
 /** @brief add a callback
@@ -131,7 +150,7 @@ void server::do_callbacks(callback_event_type event, unsigned int conn_id)
 }
 
 /// add a new connection to the server
-int server::add_connection(server_connection* newconn)
+unsigned int server::add_connection(server_connection* newconn)
 {
     unsigned int cid=next_id++;
     newconn->set_id(cid);
index 684cee8..738a869 100644 (file)
@@ -60,6 +60,7 @@ class server_connection : public connection
         server *my_server;
 
         server_connection(int _timeout);
+        virtual ~server_connection();
 
         std::ostream* get_logstream(log_level_values level);
 
@@ -100,7 +101,7 @@ class server
 
         virtual bool fill_connection_buffers(void)=0;
 
-        int add_connection(server_connection* newconn);
+        unsigned int add_connection(server_connection* newconn);
 
         void do_callbacks(callback_event_type event, unsigned int conn_id);
 
@@ -132,6 +133,8 @@ class server
         */
         virtual bool fill_buffer(long long usec_timeout=-1, long long* usec_timeout_remaining=NULL)=0;
 
+        void close();
+
         void cleanup();
 
         /** @brief get a complete data packet from any client. The packet is removed from the
index d8dc413..fca01d1 100644 (file)
@@ -66,6 +66,7 @@ socket_client_connection::socket_client_connection(int _port, const std::string&
     {
         lastErrorMsg=e.what();
         LOGSTREAM(debug,"tcp connect error: " << lastErrorMsg);
+        // FIXME: Don't call virtual function in constructor. Currently not dangerous but bad design.
         close();
     }
 
@@ -94,7 +95,7 @@ socket_client_connection::socket_client_connection(const std::string& _path,
     {
         lastErrorMsg=e.what();
         LOGSTREAM(debug,"unix connect error: " << lastErrorMsg);
-        // FIXME: Calls virtual function close in constructor
+        // FIXME: Don't call virtual function in constructor. Currently not dangerous
         close();
     }
 
@@ -107,7 +108,7 @@ socket_client_connection::socket_client_connection(const std::string& _path,
  */
 socket_client_connection::~socket_client_connection()
 {
-    close();
+    // Destructor of socket_handler will close the socket!
 }
 
 
index 4b0431a..eaad92f 100644 (file)
@@ -82,7 +82,7 @@ class socket_client_connection : public client_connection, public socket_handler
         bool fill_buffer(long long usec_timeout=-1, long long *usec_timeout_remaining=NULL)
             { return socket_handler::fill_buffer(buffer,usec_timeout,usec_timeout_remaining); }
 
-        void close();
+        virtual void close();
 
         void reconnect();
 
index 91d5899..288ec66 100644 (file)
@@ -55,6 +55,31 @@ socket_handler::socket_handler(int _sock, socket_type_value _socket_type)
 {
 }
 
+/**
+ * Destructor. Closes open socket
+ */
+socket_handler::~socket_handler()
+{
+    if (sock != -1)
+    {
+        shutdown(sock,SHUT_RDWR);
+        ::close(sock);
+
+        sock = -1;
+    }
+}
+
+/// close the underlying socket connection. Don't call directly, use the version provided
+/// by the connection class you are using.
+void socket_handler::close()
+{
+    LOGSTREAM(debug,"close connection");
+    // graceful shutdown
+    shutdown(sock,SHUT_RDWR);
+    ::close(sock);
+
+    sock = -1;
+}
 
 /// set options like fast reuse and keepalive every socket should have
 void socket_handler::set_socket_options(int sock)
@@ -90,16 +115,6 @@ void socket_handler::set_socket_options(int sock)
         EXCEPTIONSTREAM(error,t2n_communication_error,"fcntl error on socket: " << strerror(errno));
 }
 
-/// close the underlying socket connection. Don't call directly, use the version provided
-/// by the connection class you are using.
-void socket_handler::close()
-{
-    LOGSTREAM(debug,"close connection");
-    // graceful shutdown
-    shutdown(sock,SHUT_RDWR);
-    ::close(sock);
-}
-
 /// is the underlying socket connection still open?
 bool socket_handler::is_closed()
 {
index b91a505..024bb9f 100644 (file)
@@ -48,7 +48,7 @@ class socket_handler
         long long write_timeout;
 
         socket_handler(int _sock, socket_type_value _socket_type);
-        // TODO: No destructor?
+        ~socket_handler();
 
         void set_socket_options(int sock);
 
index ad8bb84..a5ec5d0 100644 (file)
@@ -129,12 +129,27 @@ socket_server::socket_server(const std::string& path, mode_t filemode, const std
     start_listening();
 }
 
+/**
+ * Destructor
+ */
 socket_server::~socket_server()
 {
-    socket_handler::close();
+    // close all client connections
+    server::close();
+
+    // server socket will be closed by destructor of socket_handler
 
     if (get_type()==unix_s)
         unlink(unix_path.c_str());
+
+    // disconnect connection<->server pointer
+    std::map<unsigned int, server_connection*>::iterator it, it_end = connections.end();
+    for (it = connections.begin(); it != it_end; ++it)
+    {
+        socket_server_connection *conn = dynamic_cast<socket_server_connection*>(it->second);
+        if (conn)
+            conn->my_server = NULL;
+    }
 }
 
 /// start listening on a new server socket (called by the constructors)
@@ -272,18 +287,35 @@ void socket_server::remove_connection_socket(int sock)
     FD_CLR(sock, &connection_set);
 }
 
+/**
+ * Destructor
+ */
+socket_server_connection::~socket_server_connection()
+{
+    // Only notify parent server about going down.
+    // The real socket will be closed by the destructor of the base classes.
+    if (my_server && sock != -1)
+    {
+        socket_server *srv = dynamic_cast<socket_server*>(my_server);
+        if (srv)
+            srv->remove_connection_socket(sock);
+    }
+}
+
 /// close this connection. complete data waiting in the buffer can still be retrieved.
 void socket_server_connection::close()
 {
-    if (!server_connection::is_closed())
+    if (my_server && sock != -1)
     {
-        socket_handler::close();
-        server_connection::close();
+        socket_server *srv = dynamic_cast<socket_server*>(my_server);
+        if (srv)
+            srv->remove_connection_socket(sock);
     }
 
-    if (my_server)
+    if (!server_connection::is_closed())
     {
-        dynamic_cast<socket_server*>(my_server)->remove_connection_socket(sock);
+        socket_handler::close();
+        server_connection::close();
     }
 }
 
index 4c928c5..360777e 100644 (file)
@@ -79,6 +79,8 @@ class socket_server_connection : public socket_handler, public server_connection
            : server_connection(_timeout), socket_handler(_sock,_stype)
            { }
 
+        ~socket_server_connection();
+
         std::ostream* get_logstream(log_level_values level)
             { return server_connection::get_logstream(level); }
 
@@ -89,7 +91,7 @@ class socket_server_connection : public socket_handler, public server_connection
         bool fill_buffer(long long usec_timeout=-1,long long* usec_timeout_remaining=NULL)
             { return socket_handler::fill_buffer(buffer,usec_timeout,usec_timeout_remaining); }
 
-        void close();
+        virtual void close();
 };
 
 }
index c5ec6d2..59aa6ae 100644 (file)
@@ -164,6 +164,9 @@ class test_newserver : public TestFixture
             default:
             // parent
             {
+                // don't kill us on broken pipe
+                signal(SIGPIPE, SIG_IGN);
+
                 // wait till server is up
                 sleep(1);
                 socket_client_connection sc("./socket");
@@ -188,14 +191,7 @@ class test_newserver : public TestFixture
                 catch(...)
                     { throw; }
 
-
-                // FIXME: This test won't work by design:
-                // The server closes the file descriptor
-                // and then reopens the server socket.
-                // Unfortunately the same socket # will be assigned
-                // and so the second write succeedes, too.
-//                CPPUNIT_ASSERT_EQUAL(string("error reading from socket : Connection reset by peer"),errormsg);
-                CPPUNIT_ASSERT_EQUAL(string(""),errormsg);
+                CPPUNIT_ASSERT_EQUAL(string("write() returned Bad file descriptor"),errormsg);
             }
         }
     }
index c08df62..7187ffe 100644 (file)
@@ -47,7 +47,7 @@ string testfunc(const string& str)
 
     // call handle, eventually reentrant
     if (global_server)
-        global_server->handle(1000);
+        global_server->handle(10000);
 
     return ret;
 }
@@ -124,8 +124,6 @@ class test_reentrant : public TestFixture
 
     CPPUNIT_TEST_SUITE_END();
 
-    pid_t child_pid;
-
     public:
 
     void setUp()
@@ -136,7 +134,7 @@ class test_reentrant : public TestFixture
 
     void ReentrantServer()
     {
-        switch(child_pid=fork())
+        switch(fork())
         {
             case -1:
             {
@@ -154,17 +152,31 @@ class test_reentrant : public TestFixture
                 fork();
                 fork();
 
-                for (int i=0; i < 100; i++)
+                try
                 {
-                    socket_client_connection sc("./socket");
-                    command_client cc(&sc);
-
-                    result_container rc;
-                    cc.send_command(new testfunc_cmd("hello"),rc);
-
-                    string ret=dynamic_cast<testfunc_res*>(rc.get_result())->get_data();
-
-                    CPPUNIT_ASSERT_EQUAL(string("hello, testfunc() was here"),ret);
+                    for (int i=0; i < 100; i++)
+                    {
+                        socket_client_connection sc("./socket");
+                        command_client cc(&sc);
+
+                        result_container rc;
+                        cc.send_command(new testfunc_cmd("hello"),rc);
+
+                        testfunc_res *res = dynamic_cast<testfunc_res*>(rc.get_result());
+                        if (res)
+                        {
+                            string ret = res->get_data();
+                            if (ret != "hello, testfunc() was here")
+                                std::cout << "ERROR reentrant server testfunc_res failed, res: " << ret << "\n";
+                        }
+                        else
+                        {
+                            std::cout << "ERROR result from reentrant server empty\n";
+                        }
+                    }
+                } catch (exception &e)
+                {
+                    cerr << "caught exception: " << e.what() << endl;
                 }
 
                 // don't call atexit and stuff
@@ -174,16 +186,29 @@ class test_reentrant : public TestFixture
             default:
             // parent
             {
+                // don't kill us on broken pipe
+                signal(SIGPIPE, SIG_IGN);
+
                 socket_server ss("./socket");
                 command_server cs(ss);
 
                 global_server=&cs;
 
                 // max 10 sec
-                long long maxtime=5000000;
+                long long maxtime=1000000;
                 while(maxtime > 0)
                     cs.handle(maxtime,&maxtime);
-                    
+
+                // max 10 sec
+                maxtime=1000000;
+                while(maxtime > 0)
+                    cs.handle(maxtime,&maxtime);
+
+                // max 10 sec
+                maxtime=1000000;
+                while(maxtime > 0)
+                    cs.handle(maxtime,&maxtime);
+
                 global_server = NULL;
             }
 
index db31fba..b7f3248 100644 (file)
@@ -122,7 +122,8 @@ class test_serialize : public TestFixture
     public:
 
     void setUp()
-    { }
+    {
+    }
 
     void tearDown()
     {
index 36e08f1..1c8e09e 100644 (file)
@@ -284,9 +284,10 @@ class test_simplecmd : public TestFixture
                 socket_server ss("./socket");
                 command_server cs(ss);
 
-                // max 10 sec
-                for (int i=0; i < 10; i++)
+                // max 60 sec - we need atleast 28 handle calls to transfer the buffer
+                for (int i=0; i < 60; i++) {
                     cs.handle(1000000);
+                }
 
                 // don't call atexit and stuff
                 _exit(0);
index e194b85..5156869 100644 (file)
@@ -512,6 +512,7 @@ class test_timeout : public TestFixture
 
                 // bail out as soon as we get something
                 ss.fill_buffer(-1);
+                ss.fill_buffer(-1);
                 // don't call atexit and stuff
                 _exit(0);
             }