From: Thomas Jarosch Date: Tue, 25 Nov 2008 18:45:41 +0000 (+0000) Subject: libt2n: (tomj) fixed call of virtual function close() from destructor, fixed return... X-Git-Tag: v0.5~6 X-Git-Url: http://developer.intra2net.com/git/?p=libt2n;a=commitdiff_plain;h=56f3994d74dbc36d10bfa83b50b016bf269ac563 libt2n: (tomj) fixed call of virtual function close() from destructor, fixed return value of server::add_connection() --- diff --git a/src/command_client.hxx b/src/command_client.hxx index 59d4755..49f2819 100644 --- a/src/command_client.hxx +++ b/src/command_client.hxx @@ -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 constructorException; public: diff --git a/src/command_server.cpp b/src/command_server.cpp index e41d41b..8c4bb42 100644 --- a/src/command_server.cpp +++ b/src/command_server.cpp @@ -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) diff --git a/src/command_server.hxx b/src/command_server.hxx index 04508b5..97b3a07 100644 --- a/src/command_server.hxx +++ b/src/command_server.hxx @@ -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); diff --git a/src/connection.cpp b/src/connection.cpp index cc543b2..4abfad4 100644 --- a/src/connection.cpp +++ b/src/connection.cpp @@ -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); } diff --git a/src/server.cpp b/src/server.cpp index 30bf7d3..749f4fb 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -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::iterator ie=connections.end(); for(std::map::iterator i=connections.begin(); i != ie; i++) delete i->second; + + connections.clear(); +} + +/** + * Close all open connections + */ +void server::close() +{ + std::map::iterator ie=connections.end(); + for(std::map::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); diff --git a/src/server.hxx b/src/server.hxx index 684cee8..738a869 100644 --- a/src/server.hxx +++ b/src/server.hxx @@ -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 diff --git a/src/socket_client.cpp b/src/socket_client.cpp index d8dc413..fca01d1 100644 --- a/src/socket_client.cpp +++ b/src/socket_client.cpp @@ -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! } diff --git a/src/socket_client.hxx b/src/socket_client.hxx index 4b0431a..eaad92f 100644 --- a/src/socket_client.hxx +++ b/src/socket_client.hxx @@ -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(); diff --git a/src/socket_handler.cpp b/src/socket_handler.cpp index 91d5899..288ec66 100644 --- a/src/socket_handler.cpp +++ b/src/socket_handler.cpp @@ -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() { diff --git a/src/socket_handler.hxx b/src/socket_handler.hxx index b91a505..024bb9f 100644 --- a/src/socket_handler.hxx +++ b/src/socket_handler.hxx @@ -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); diff --git a/src/socket_server.cpp b/src/socket_server.cpp index ad8bb84..a5ec5d0 100644 --- a/src/socket_server.cpp +++ b/src/socket_server.cpp @@ -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::iterator it, it_end = connections.end(); + for (it = connections.begin(); it != it_end; ++it) + { + socket_server_connection *conn = dynamic_cast(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(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(my_server); + if (srv) + srv->remove_connection_socket(sock); } - if (my_server) + if (!server_connection::is_closed()) { - dynamic_cast(my_server)->remove_connection_socket(sock); + socket_handler::close(); + server_connection::close(); } } diff --git a/src/socket_server.hxx b/src/socket_server.hxx index 4c928c5..360777e 100644 --- a/src/socket_server.hxx +++ b/src/socket_server.hxx @@ -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(); }; } diff --git a/test/newserver.cpp b/test/newserver.cpp index c5ec6d2..59aa6ae 100644 --- a/test/newserver.cpp +++ b/test/newserver.cpp @@ -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); } } } diff --git a/test/reentrant.cpp b/test/reentrant.cpp index c08df62..7187ffe 100644 --- a/test/reentrant.cpp +++ b/test/reentrant.cpp @@ -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(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(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; } diff --git a/test/serialize.cpp b/test/serialize.cpp index db31fba..b7f3248 100644 --- a/test/serialize.cpp +++ b/test/serialize.cpp @@ -122,7 +122,8 @@ class test_serialize : public TestFixture public: void setUp() - { } + { + } void tearDown() { diff --git a/test/simplecmd.cpp b/test/simplecmd.cpp index 36e08f1..1c8e09e 100644 --- a/test/simplecmd.cpp +++ b/test/simplecmd.cpp @@ -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); diff --git a/test/timeout.cpp b/test/timeout.cpp index e194b85..5156869 100644 --- a/test/timeout.cpp +++ b/test/timeout.cpp @@ -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); }