From 19cb22334def76d96205c3fc9b10b0c606368760 Mon Sep 17 00:00:00 2001 From: Steve Huston Date: Fri, 27 Aug 2010 19:48:15 +0000 Subject: ChangeLogTag:Fri Aug 27 19:17:11 UTC 2010 Steve Huston --- ChangeLog | 32 ++++++++++++++++++ ace/ACE.cpp | 41 ++++++++++++----------- ace/Acceptor.cpp | 21 ++---------- ace/SOCK_Dgram.cpp | 88 +++++++------------------------------------------- ace/SOCK_IO.cpp | 29 +---------------- tests/MT_SOCK_Test.cpp | 71 ++++++++++++++-------------------------- tests/SOCK_Test.cpp | 53 +++++++++--------------------- tests/tests.mpc | 8 +++++ 8 files changed, 118 insertions(+), 225 deletions(-) diff --git a/ChangeLog b/ChangeLog index bfb4621f09a..e8e7db181e0 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,35 @@ +Fri Aug 27 19:17:11 UTC 2010 Steve Huston + + * ace/ACE.cpp (handle_ready, handle_timed_complete, + handle_timed_accept): On platforms where poll() is + available, prefer that to using select() for checking a single + handle's state and/or waiting for a condition. This preference + was previously only used if ACE_HAS_LIMITED_SELECT was set. The + ACE_HAS_LIMITED_SELECT choice is removed, making ACE_HAS_POLL the + setting that switches this preference. The driving reason for this + is that if select() is called to detect changes on a handle whose + values falls outside that which can safely be stored in an fdset, + the handle-setting macros/functions will set/clear bits outside + of the fdset. This results in very weird memory changes, often in + the stack, which are very hard to diagnose. poll()'s operation + does not suffer from this affect. With the growing use of large + numbers of handles and use of ACE_Dev_Poll_Reactor on Linux, + the rate at which this problem was cropping up was increasing. + Thanks to Olivier Langlois for diagnosing this problem and + proposing the patch. + + (handle_ready): Properly set the poll condition for read and/or + write. Thanks to kumaran.prem@gmail.com for this fix. + + * ace/Acceptor.cpp: + * ace/SOCK_IO.cpp: + * ace/SOCK_Dgram.cpp: + * tests/MT_SOCK_Test.cpp: + * tests/SOCK_Test.cpp: Replaced use of ACE_OS::select() with + ACE::handle_ready() and friends. + + This all resolves Bugzilla #3606. + Fri Aug 27 15:01:41 UTC 2010 Steve Huston * Connector.{h cpp} (ACE_NonBlocking_Connect_Handler): Add a diff --git a/ace/ACE.cpp b/ace/ACE.cpp index 12a55194c3d..0d4d9b80300 100644 --- a/ace/ACE.cpp +++ b/ace/ACE.cpp @@ -33,9 +33,9 @@ extern "C" int maxFiles; #include "ace/ACE.inl" #endif /* __ACE_INLINE__ */ -#if defined (ACE_HAS_POLL) && defined (ACE_HAS_LIMITED_SELECT) +#if defined (ACE_HAS_POLL) # include "ace/OS_NS_poll.h" -#endif /* ACE_HAS_POLL && ACE_HAS_LIMITED_SELECT */ +#endif /* ACE_HAS_POLL */ ACE_RCSID (ace, @@ -2194,14 +2194,19 @@ ACE::handle_ready (ACE_HANDLE handle, int write_ready, int exception_ready) { -#if defined (ACE_HAS_POLL) && defined (ACE_HAS_LIMITED_SELECT) - ACE_UNUSED_ARG (write_ready); +#if defined (ACE_HAS_POLL) ACE_UNUSED_ARG (exception_ready); struct pollfd fds; fds.fd = handle; - fds.events = read_ready ? POLLIN : POLLOUT; + fds.events = read_ready ? POLLIN : 0; + + if( write_ready ) + { + fds.events |= POLLOUT; + } + fds.revents = 0; int result = ACE_OS::poll (&fds, 1, timeout); @@ -2224,7 +2229,7 @@ ACE::handle_ready (ACE_HANDLE handle, exception_ready ? handle_set.fdset () : 0, // exception_fds. timeout); -#endif /* ACE_HAS_POLL && ACE_HAS_LIMITED_SELECT */ +#endif /* ACE_HAS_POLL */ switch (result) { @@ -2533,7 +2538,7 @@ ACE::handle_timed_complete (ACE_HANDLE h, { ACE_TRACE ("ACE::handle_timed_complete"); -#if !defined (ACE_WIN32) && defined (ACE_HAS_POLL) && defined (ACE_HAS_LIMITED_SELECT) +#if !defined (ACE_WIN32) && defined (ACE_HAS_POLL) struct pollfd fds; @@ -2546,7 +2551,7 @@ ACE::handle_timed_complete (ACE_HANDLE h, ACE_Handle_Set wr_handles; rd_handles.set_bit (h); wr_handles.set_bit (h); -#endif /* !ACE_WIN32 && ACE_HAS_POLL && ACE_HAS_LIMITED_SELECT */ +#endif /* !ACE_WIN32 && ACE_HAS_POLL */ #if defined (ACE_WIN32) // Winsock is different - it sets the exception bit for failed connect, @@ -2566,7 +2571,7 @@ ACE::handle_timed_complete (ACE_HANDLE h, ex_handles, timeout); #else -# if defined (ACE_HAS_POLL) && defined (ACE_HAS_LIMITED_SELECT) +# if defined (ACE_HAS_POLL) int n = ACE_OS::poll (&fds, 1, timeout); @@ -2584,7 +2589,7 @@ ACE::handle_timed_complete (ACE_HANDLE h, wr_handles, 0, timeout); -# endif /* ACE_HAS_POLL && ACE_HAS_LIMITED_SELECT */ +# endif /* ACE_HAS_POLL */ #endif /* ACE_WIN32 */ // If we failed to connect within the time period allocated by the @@ -2618,18 +2623,18 @@ ACE::handle_timed_complete (ACE_HANDLE h, } #else if (is_tli) -# if defined (ACE_HAS_POLL) && defined (ACE_HAS_LIMITED_SELECT) +# if defined (ACE_HAS_POLL) need_to_check = (fds.revents & POLLIN) && !(fds.revents & POLLOUT); # else need_to_check = rd_handles.is_set (h) && !wr_handles.is_set (h); -# endif /* ACE_HAS_POLL && ACE_HAS_LIMITED_SELECT */ +# endif /* ACE_HAS_POLL */ else -# if defined (ACE_HAS_POLL) && defined (ACE_HAS_LIMITED_SELECT) +# if defined (ACE_HAS_POLL) need_to_check = (fds.revents & POLLIN); # else need_to_check = true; -# endif /* ACE_HAS_POLL && ACE_HAS_LIMITED_SELECT */ +# endif /* ACE_HAS_POLL */ #endif /* ACE_WIN32 */ if (need_to_check) @@ -2691,7 +2696,7 @@ ACE::handle_timed_accept (ACE_HANDLE listener, if (listener == ACE_INVALID_HANDLE) return -1; -#if defined (ACE_HAS_POLL) && defined (ACE_HAS_LIMITED_SELECT) +#if defined (ACE_HAS_POLL) struct pollfd fds; @@ -2703,13 +2708,13 @@ ACE::handle_timed_accept (ACE_HANDLE listener, // Use the select() implementation rather than poll(). ACE_Handle_Set rd_handle; rd_handle.set_bit (listener); -#endif /* ACE_HAS_POLL && ACE_HAS_LIMITED_SELECT */ +#endif /* ACE_HAS_POLL */ // We need a loop here if is enabled. for (;;) { -#if defined (ACE_HAS_POLL) && defined (ACE_HAS_LIMITED_SELECT) +#if defined (ACE_HAS_POLL) int n = ACE_OS::poll (&fds, 1, timeout); @@ -2725,7 +2730,7 @@ ACE::handle_timed_accept (ACE_HANDLE listener, int n = ACE_OS::select (select_width, rd_handle, 0, 0, timeout); -#endif /* ACE_HAS_POLL && ACE_HAS_LIMITED_SELECT */ +#endif /* ACE_HAS_POLL */ switch (n) { diff --git a/ace/Acceptor.cpp b/ace/Acceptor.cpp index b66b293eef1..e805094bb90 100644 --- a/ace/Acceptor.cpp +++ b/ace/Acceptor.cpp @@ -8,12 +8,10 @@ #endif /* ACE_LACKS_PRAGMA_ONCE */ #include "ace/Acceptor.h" -#include "ace/Handle_Set.h" #include "ace/Svc_Handler.h" #include "ace/WFMO_Reactor.h" #include "ace/OS_NS_stdio.h" #include "ace/OS_NS_string.h" -#include "ace/OS_NS_sys_select.h" ACE_RCSID (ace, Acceptor, @@ -367,17 +365,9 @@ template int ACE_Acceptor::handle_input (ACE_HANDLE listener) { ACE_TRACE ("ACE_Acceptor::handle_input"); - ACE_Handle_Set conn_handle; // Default is "timeout (0, 0)," which means "poll." ACE_Time_Value timeout; -# if defined (ACE_WIN32) - // This arg is ignored on Windows and causes pointer truncation - // warnings on 64-bit compiles - int select_width = 0; -# else - int select_width = int (listener) + 1; -# endif /* ACE_WIN32 */ // Accept connections from clients. Note that a loop is used for two // reasons: @@ -434,18 +424,13 @@ ACE_Acceptor::handle_input (ACE_HANDLE listene ACE_TEXT ("activate_svc_handler"))); return 0; } - - conn_handle.set_bit (listener); } // Now, check to see if there is another connection pending and // break out of the loop if there is none. - while (this->use_select_ - && ACE_OS::select (select_width, - conn_handle, - 0, - 0, - &timeout) == 1); + while (this->use_select_ && + ACE::handle_read_ready (listener, + &timeout) == 1); return 0; } diff --git a/ace/SOCK_Dgram.cpp b/ace/SOCK_Dgram.cpp index e8b0108747f..f3aea4fee2f 100644 --- a/ace/SOCK_Dgram.cpp +++ b/ace/SOCK_Dgram.cpp @@ -1,12 +1,10 @@ #include "ace/SOCK_Dgram.h" -#include "ace/Handle_Set.h" #include "ace/Log_Msg.h" #include "ace/INET_Addr.h" #include "ace/ACE.h" #include "ace/OS_NS_string.h" #include "ace/OS_Memory.h" -#include "ace/OS_NS_sys_select.h" #include "ace/OS_NS_ctype.h" #include "ace/os_include/net/os_if.h" #include "ace/Truncate.h" @@ -54,34 +52,9 @@ ACE_SOCK_Dgram::recv (iovec *io_vec, { ACE_TRACE ("ACE_SOCK_Dgram::recv"); #if defined (FIONREAD) - ACE_Handle_Set handle_set; - handle_set.reset (); - handle_set.set_bit (this->get_handle ()); - - // Check the status of the current socket to make sure there's data - // to recv (or time out). -# if defined (ACE_WIN32) - // This arg is ignored on Windows and causes pointer truncation - // warnings on 64-bit compiles. - int select_width = 0; -# else - int select_width = int (this->get_handle ()) + 1; -# endif /* ACE_WIN32 */ - switch (ACE_OS::select (select_width, - handle_set, - 0, 0, - timeout)) + if( ACE::handle_read_ready (this->get_handle (), timeout) != 1 ) { - case -1: return -1; - /* NOTREACHED */ - case 0: - errno = ETIME; - return -1; - /* NOTREACHED */ - default: - // Goes fine, fallthrough to get data - break; } sockaddr *saddr = (sockaddr *) addr.get_addr (); @@ -453,35 +426,15 @@ ACE_SOCK_Dgram::recv (void *buf, int flags, const ACE_Time_Value *timeout) const { - ACE_Handle_Set handle_set; - handle_set.reset (); - handle_set.set_bit (this->get_handle ()); - - // Check the status of the current socket. -#if defined (ACE_WIN32) - // This arg is ignored on Windows and causes pointer truncation - // warnings on 64-bit compiles. - int select_width = 0; -#else - int select_width = int (this->get_handle ()) + 1; -#endif /* ACE_WIN32 */ - switch (ACE_OS::select (select_width, - handle_set, - 0, - 0, - timeout)) + if( ACE::handle_read_ready (this->get_handle (), timeout) == 1 ) { - case -1: - return -1; - /* NOTREACHED */ - case 0: - errno = ETIME; - return -1; - /* NOTREACHED */ - default: // Goes fine, call to get data return this->recv (buf, n, addr, flags); } + else + { + return -1; + } } ssize_t @@ -491,35 +444,16 @@ ACE_SOCK_Dgram::send (const void *buf, int flags, const ACE_Time_Value *timeout) const { - ACE_Handle_Set handle_set; - handle_set.reset (); - handle_set.set_bit (this->get_handle ()); - // Check the status of the current socket. -#if defined (ACE_WIN32) - // This arg is ignored on Windows and causes pointer truncation - // warnings on 64-bit compiles. - int select_width = 0; -#else - int select_width = int (this->get_handle ()) + 1; -#endif /* ACE_WIN32 */ - switch (ACE_OS::select (select_width, - 0, - handle_set, - 0, - timeout)) + if( ACE::handle_write_ready (this->get_handle (), timeout) == 1 ) { - case -1: - return -1; - /* NOTREACHED */ - case 0: - errno = ETIME; - return -1; - /* NOTREACHED */ - default: // Goes fine, call to transmit the data. return this->send (buf, n, addr, flags); } + else + { + return -1; + } } int diff --git a/ace/SOCK_IO.cpp b/ace/SOCK_IO.cpp index 36a7d171456..31aaaa00dea 100644 --- a/ace/SOCK_IO.cpp +++ b/ace/SOCK_IO.cpp @@ -2,8 +2,6 @@ #include "ace/SOCK_IO.h" -#include "ace/Handle_Set.h" -#include "ace/OS_NS_sys_select.h" #include "ace/OS_NS_sys_socket.h" #include "ace/OS_Memory.h" #include "ace/Truncate.h" @@ -37,35 +35,10 @@ ACE_SOCK_IO::recvv (iovec *io_vec, { ACE_TRACE ("ACE_SOCK_IO::recvv"); #if defined (FIONREAD) - ACE_Handle_Set handle_set; - handle_set.reset (); - handle_set.set_bit (this->get_handle ()); - io_vec->iov_base = 0; - - // Check the status of the current socket. -# if defined (ACE_WIN32) - // This arg is ignored on Windows and causes pointer truncation - // warnings on 64-bit compiles. - int select_width = 0; -# else - int select_width = int (this->get_handle ()) + 1; -# endif /* ACE_WIN32 */ - switch (ACE_OS::select (select_width, - handle_set, - 0, 0, - timeout)) + if( ACE::handle_read_ready (this->get_handle (), timeout) != 1 ) { - case -1: - return -1; - /* NOTREACHED */ - case 0: - errno = ETIME; return -1; - /* NOTREACHED */ - default: - // Goes fine, fallthrough to get data - break; } int inlen = 0; diff --git a/tests/MT_SOCK_Test.cpp b/tests/MT_SOCK_Test.cpp index ea12c5f310b..638d5e81613 100644 --- a/tests/MT_SOCK_Test.cpp +++ b/tests/MT_SOCK_Test.cpp @@ -24,7 +24,6 @@ // ============================================================================ #include "test_config.h" -#include "ace/OS_NS_sys_select.h" #include "ace/OS_NS_sys_wait.h" #include "ace/OS_NS_unistd.h" #include "ace/Thread.h" @@ -168,7 +167,6 @@ server (void *arg) // calls... ACE_SOCK_Stream new_stream; ACE_INET_Addr cli_addr; - ACE_Handle_Set handle_set; const ACE_Time_Value def_timeout (ACE_DEFAULT_TIMEOUT); ACE_Time_Value tv (def_timeout); @@ -185,42 +183,34 @@ server (void *arg) { char buf[BUFSIZ]; - handle_set.reset (); - handle_set.set_bit (peer_acceptor->get_handle ()); - ACE_DEBUG((LM_DEBUG, "(%P|%t) server: Waiting for connection...\n")); - int select_width; -# if defined (ACE_WIN64) - // This arg is ignored on Windows and causes pointer truncation - // warnings on 64-bit compiles. - select_width = 0; -# else - select_width = int (peer_acceptor->get_handle ()) + 1; -# endif /* ACE_WIN64 */ - int result = ACE_OS::select (select_width, handle_set, 0, 0, &tv); + int result = ACE::handle_read_ready (peer_acceptor->get_handle (), &tv); ACE_ASSERT (tv == def_timeout); if (result == -1) - ACE_ERROR_RETURN ((LM_ERROR, - ACE_TEXT ("(%P|%t) %p\n"), - ACE_TEXT ("server: select acceptor")), - 0); - else if (result == 0) { - ACE_DEBUG ((LM_DEBUG, - ACE_TEXT ("(%P|%t) server: Test finished.\n"))); - // The meaning of the backlog parameter for listen() varies by - // platform. For some reason lost to history, the specified value - // is typically backlog * 1.5, backlog * 1.5 + 1, or event taken - // literally as on Windows. We'll accept any number less than - // backlog * 2 as valid. - if (num_clients_connected >= BACKLOG * 2) - ACE_ERROR ((LM_ERROR, - ACE_TEXT ("(%P|%t) server: Incorrect # client ") - ACE_TEXT ("connections. Expected:%d-%d Actual:%d\n"), - BACKLOG, BACKLOG * 2, num_clients_connected)); - return 0; + if (errno == ETIME) + { + ACE_DEBUG ((LM_DEBUG, + ACE_TEXT ("(%P|%t) server: Test finished.\n"))); + // The meaning of the backlog parameter for listen() varies by + // platform. For some reason lost to history, the specified value + // is typically backlog * 1.5, backlog * 1.5 + 1, or event taken + // literally as on Windows. We'll accept any number less than + // backlog * 2 as valid. + if (num_clients_connected > BACKLOG * 2) + ACE_ERROR ((LM_ERROR, + ACE_TEXT ("(%P|%t) server: Incorrect # client ") + ACE_TEXT ("connections. Expected:%d-%d Actual:%d\n"), + BACKLOG, BACKLOG * 2, num_clients_connected)); + return 0; + } + + ACE_ERROR_RETURN ((LM_ERROR, + ACE_TEXT ("(%P|%t) %p\n"), + ACE_TEXT ("server: handle_read_ready acceptor")), + 0); } // Create a new ACE_SOCK_Stream endpoint (note automatic restart @@ -244,29 +234,16 @@ server (void *arg) ACE_TEXT ("(%P|%t) %p\n"), ACE_TEXT ("server: enable non blocking i/o")), 0); - handle_set.reset (); - handle_set.set_bit (new_stream.get_handle ()); - // Read data from client (terminate on error). ACE_DEBUG ((LM_DEBUG, ACE_TEXT ("(%P|%t) server: Waiting for data...\n"))); for (ssize_t r_bytes; ;) { - int select_width; -# if defined (ACE_WIN64) - // This arg is ignored on Windows and causes pointer truncation - // warnings on 64-bit compiles. - select_width = 0; -# else - select_width = int (new_stream.get_handle ()) + 1; -# endif /* ACE_WIN64 */ - if (ACE_OS::select (select_width, - handle_set, - 0, 0, 0) == -1) + if (ACE::handle_read_ready (new_stream.get_handle (), 0) == -1) ACE_ERROR_RETURN ((LM_ERROR, ACE_TEXT ("(%P|%t) %p\n"), - ACE_TEXT ("select")), + ACE_TEXT ("stream handle_read_ready")), 0); ACE_DEBUG ((LM_DEBUG, "(%P|%t) server: Receiving data...\n")); diff --git a/tests/SOCK_Test.cpp b/tests/SOCK_Test.cpp index 34000f53eee..2fa4e1f36e3 100644 --- a/tests/SOCK_Test.cpp +++ b/tests/SOCK_Test.cpp @@ -22,7 +22,6 @@ #include "test_config.h" #include "ace/OS_NS_unistd.h" -#include "ace/OS_NS_sys_select.h" #include "ace/OS_NS_sys_wait.h" #include "ace/Thread.h" #include "ace/Time_Value.h" @@ -69,6 +68,17 @@ client (void *arg) if (cli_stream.disable (ACE_NONBLOCK) == -1) ACE_ERROR ((LM_ERROR, ACE_TEXT ("(%P|%t) %p\n"), ACE_TEXT ("disable"))); + // Test Bug 3606 + const ACE_Time_Value def_timeout (ACE_DEFAULT_TIMEOUT); + ACE_Time_Value tv (def_timeout); + int result = ACE::handle_ready (cli_stream.get_handle (), &tv, + 1, // read_ready + 1, // write_ready + 0); + // we expect the handle to be at leat write_ready since it is freshly connected. + if (result == -1) + ACE_ERROR_RETURN ((LM_ERROR, ACE_TEXT ("(%P|%t) %p\n"), ACE_TEXT ("ACE::handle_ready")), 0); + // Send data to server (correctly handles "incomplete writes"). for (const char *c = ACE_ALPHABET; *c != '\0'; c++) @@ -104,36 +114,18 @@ server (void *arg) // calls... ACE_SOCK_Stream new_stream; ACE_INET_Addr cli_addr; - ACE_Handle_Set handle_set; const ACE_Time_Value def_timeout (ACE_DEFAULT_TIMEOUT); ACE_Time_Value tv (def_timeout); char buf[BUFSIZ]; const char *t = ACE_ALPHABET; - handle_set.reset (); - handle_set.set_bit (peer_acceptor->get_handle ()); - - int select_width; -# if defined (ACE_WIN64) - // This arg is ignored on Windows and causes pointer truncation - // warnings on 64-bit compiles. - select_width = 0; -# else - select_width = int (peer_acceptor->get_handle ()) + 1; -# endif /* ACE_WIN64 */ - int result = ACE_OS::select (select_width, - handle_set, - 0, 0, &tv); + int result = ACE::handle_read_ready (peer_acceptor->get_handle (), &tv); + ACE_ASSERT (tv == def_timeout); if (result == -1) - ACE_ERROR_RETURN ((LM_ERROR, ACE_TEXT ("(%P|%t) %p\n"), ACE_TEXT ("select")), 0); - else if (result == 0) - { - ACE_DEBUG ((LM_DEBUG, ACE_TEXT ("(%P|%t) select timed out, shutting down\n"))); - return 0; - } + ACE_ERROR_RETURN ((LM_ERROR, ACE_TEXT ("(%P|%t) %p\n"), ACE_TEXT ("handle_read_ready")), 0); // Create a new ACE_SOCK_Stream endpoint (note automatic restart // if errno == EINTR). @@ -147,24 +139,11 @@ server (void *arg) if (new_stream.enable (ACE_NONBLOCK) == -1) ACE_ERROR_RETURN ((LM_ERROR, ACE_TEXT ("(%P|%t) %p\n"), ACE_TEXT ("enable")), 0); - handle_set.reset (); - handle_set.set_bit (new_stream.get_handle ()); - // Read data from client (terminate on error). - int select_width; for (ssize_t r_bytes; ;) { -# if defined (ACE_WIN64) - // This arg is ignored on Windows and causes pointer truncation - // warnings on 64-bit compiles. - select_width = 0; -# else - select_width = int (new_stream.get_handle ()) + 1; -# endif /* ACE_WIN64 */ - if (ACE_OS::select (select_width, - handle_set, - 0, 0, 0) == -1) - ACE_ERROR_RETURN ((LM_ERROR, ACE_TEXT ("(%P|%t) %p\n"), ACE_TEXT ("select")), 0); + if (ACE::handle_read_ready (new_stream.get_handle (), 0) == -1) + ACE_ERROR_RETURN ((LM_ERROR, ACE_TEXT ("(%P|%t) %p\n"), ACE_TEXT ("handle_read_ready")), 0); while ((r_bytes = new_stream.recv (buf, 1)) > 0) { diff --git a/tests/tests.mpc b/tests/tests.mpc index 16f69453a1d..1fd4cdc06a8 100644 --- a/tests/tests.mpc +++ b/tests/tests.mpc @@ -456,6 +456,14 @@ project(Conn Test) : acetest { } } +project(Connector Test) : acetest { + avoids += ace_for_tao + exename = Connector_Test + Source_Files { + Connector_Test.cpp + } +} + project(Date Time Test) : acetest { avoids += ace_for_tao exename = Date_Time_Test -- cgit v1.2.1