Timeout support - Please review

Hi,

Any and all occurrences of g_cond_wait() has the potential
to block
forever if the remote server is physically disconnected or
powered of.

The blocking behavior can be demonstrated (with pseudo code)
like this:

1) Get a server object reference:

   CORBA_ORB orb = get_orb();
   char *objref = get_corbaloc_str();
   CORBA_Object obj =
(CORBA_Object)CORBA_ORB_string_to_object(orb, objref, ev);

2) Invoke some method on the server object:

   CORBA_Environment ev[1];
   CORBA_exception_init(ev);
   FOO_INTERFACE_method(obj, <arguments>, ev); 

The above operation will block forever in
giop_recv_buffer_get() if the
remote server has been physically powered down. It
immediately throws an
exception if the server object is merely dead or
non-existent. 

As a rule of thumb - Blocking forever is generally bad
performance-wise
for the client application...

The appended patch therefore implements support for the
ex_CORBA_TIMEOUT
system exception by replacing most occurrences of
g_cond_wait() with
g_cond_timed_wait(). The only remaining instance of
g_cond_wait() is
found in link_exec_command(). I haven't observed any
problems with
link_exec_command() so I've not touched that code.

The affected code is:

1) New ORB option: GIOPTimeoutLimit - timeout limit in
microseconds.
Defaults to 30 seconds.

2) giop_recv_buffer_get(). This place is obvious. We do not
want to wait
indefinitely for data to be received from a downed host. The
waiting
interval is configurable with the new GIOPTimeoutLimit ORB
option.

3) Any code invoking link_wait():

   * link_connection_wait_connected_T()
   * link_connection_try_reconnect()
   * giop_connection_try_reconnect(), calls
link_connection_try_reconnect()
   * ORBit_try_connection_T(), calls
giop_connection_try_reconnect()
   * ORBit_object_get_connection(), calls
ORBit_try_connection_T()

4) The waiting time in link_wait() is set at compile time to
LINK_WAIT_TIMEOUT_USEC which is presently 10 seconds.

5) link_wait() has been modified to return a gboolean. FALSE
if the
timeout expired, TRUE otherwise.

6) link_connection_wait_connected_T() and
link_connection_try_reconnect() will both disconnect the
connection if
link_wait() experienced a timeout (returned FALSE).

Please review and test this patch. I'm not entirely
convinced that I
haven't introduced a bad bug or two but at least it works(*)
here.

Thanks,
  jules

(*) Tested with evolution-brutus on a dual Opteron Gentoo.

Index: ChangeLog
============================================================
=======
RCS file: /cvs/gnome/ORBit2/ChangeLog,v
retrieving revision 1.778
diff -u -p -r1.778 ChangeLog
--- ChangeLog	22 Nov 2006 20:34:39 -0000	1.778
+++ ChangeLog	4 Dec 2006 13:20:48 -0000
 -1,3
+1,25 
+2006-12-04  Jules Colding  <coldingomesc.com>
+
+	* configure.in: Added autoconf 2.60+ required datarootdir
variable
+
+2006-12-03  Jules Colding  <coldingomesc.com>
+
+	* include/orbit/GIOP/giop.h: Declare
giop_recv_set_timeout().
+
+	* src/orb/orb-core/corba-orb.c (CORBA_ORB_init): Set GIOP
timeout 
+	limit from the ORB option.
+	Added new ORB option - GIOPTimeoutLimit.
+
+	* src/orb/orb-core/orbit-small.c
(ORBit_small_invoke_stub): Throw a 
+	TIMEOUT exception if a reply hasn't been recieved within
the GIOP
+	timeout limit.
+
+	* src/orb/GIOP/giop-recv-buffer.c (giop_recv_buffer_get):
Replaced 
+	a g_cond_wait() with a g_cond_timed_wait(). The original
g_cond_wait()
+	could potentially block forever if a remote server was
physically
+	offline.
+	(giop_recv_set_timeout): Function to adjust the GIOP
timeout limit.
+
 2006-11-22  Kjartan Maraas  <kmaraasgnome.org>

 	* ORBit-2.0.pc.in: Move MINGW_CFLAGS and LIBM to
Libs.private
Index: configure.in
============================================================
=======
RCS file: /cvs/gnome/ORBit2/configure.in,v
retrieving revision 1.175
diff -u -p -r1.175 configure.in
--- configure.in	22 Nov 2006 20:34:41 -0000	1.175
+++ configure.in	4 Dec 2006 13:20:48 -0000
 -37,6
+37,9  AM_CONFIG_HEADER(config.h)
 dnl Initialize automake stuff
 AM_INIT_AUTOMAKE(ORBit2, $ORBIT_VERSION, no-define)

+dnl Required by autoconf 2.60
+AC_SUBST(datarootdir)
+
 AC_CANONICAL_HOST
 AC_MSG_CHECKING([for Win32])
 case "$host" in
Index: include/orbit/GIOP/giop-recv-buffer.h
============================================================
=======
RCS file:
/cvs/gnome/ORBit2/include/orbit/GIOP/giop-recv-buffer.h,v
retrieving revision 1.33
diff -u -p -r1.33 giop-recv-buffer.h
--- include/orbit/GIOP/giop-recv-buffer.h	14 Jan 2004
11:04:01 -0000	1.33
+++ include/orbit/GIOP/giop-recv-buffer.h	4 Dec 2006
13:20:48 -0000
 -58,7
+58,8  void            giop_recv_list_setup_que
 void            giop_recv_list_setup_queue_entry_async
(GIOPMessageQueueEntry *ent,
 							GIOPAsyncCallback      cb);

-GIOPRecvBuffer *giop_recv_buffer_get              
(GIOPMessageQueueEntry *ent);
+GIOPRecvBuffer *giop_recv_buffer_get              
(GIOPMessageQueueEntry *ent,
+						    gboolean *timeout);
 void            giop_recv_buffer_unuse            
(GIOPRecvBuffer        *buf);

 #define giop_recv_buffer_reply_status(buf) (               

Index: include/orbit/GIOP/giop-types.h
============================================================
=======
RCS file:
/cvs/gnome/ORBit2/include/orbit/GIOP/giop-types.h,v
retrieving revision 1.21
diff -u -p -r1.21 giop-types.h
--- include/orbit/GIOP/giop-types.h	27 Oct 2003 16:14:12
-0000	1.21
+++ include/orbit/GIOP/giop-types.h	4 Dec 2006 13:20:48
-0000
 -35,7
+35,9  struct _GIOPThread {
 					gpointer dummy);
 };

-#define GIOP_INITIAL_MSG_SIZE_LIMIT 256*1024
+#define GIOP_INITIAL_TIMEOUT_LIMIT (30000000) /* 30 seconds
*/
+
+#define GIOP_INITIAL_MSG_SIZE_LIMIT (256*1024) /* in bytes
*/

 typedef enum {
 	GIOP_CONNECTION_SSL
Index: include/orbit/GIOP/giop.h
============================================================
=======
RCS file: /cvs/gnome/ORBit2/include/orbit/GIOP/giop.h,v
retrieving revision 1.24
diff -u -p -r1.24 giop.h
--- include/orbit/GIOP/giop.h	5 Apr 2006 07:16:10 -0000	1.24
+++ include/orbit/GIOP/giop.h	4 Dec 2006 13:20:48 -0000
 -23,6
+23,7  gboolean    giop_thread_safe       (void
 gboolean    giop_thread_io         (void);
 GIOPThread *giop_thread_self       (void);
 void        giop_invoke_async      (GIOPMessageQueueEntry
*ent);
+void        giop_recv_set_timeout  (const glong timeout);
 void        giop_recv_set_limit    (glong limit);
 glong       giop_recv_get_limit    (void);
 void        giop_incoming_signal_T (GIOPThread *tdata,
GIOPMsgType t);
 -45,6
+46,7  void        giop_thread_new_check       
 void        giop_thread_queue_process    (GIOPThread
*tdata);
 gboolean    giop_thread_queue_empty_T    (GIOPThread
*tdata);
 void        giop_thread_queue_tail_wakeup(GIOPThread
*tdata);
+

 #endif /* ORBIT2_INTERNAL_API */

Index: linc2/ChangeLog
============================================================
=======
RCS file: /cvs/gnome/ORBit2/linc2/ChangeLog,v
retrieving revision 1.262
diff -u -p -r1.262 ChangeLog
--- linc2/ChangeLog	22 Nov 2006 19:13:41 -0000	1.262
+++ linc2/ChangeLog	4 Dec 2006 13:20:49 -0000
 -1,3
+1,8 
+2006-12-04  Jules Colding  <coldingomesc.com>
+
+	* src/linc.c (link_wait): Do not wait forever for the link
+	condition to get signaled.
+
 2006-11-22  Kjartan Maraas  <kmaraasgnome.org>

 	* src/Makefile.am: Remove SSL_LIBS and SSL_CFLAGS.
Index: linc2/include/linc/linc.h
============================================================
=======
RCS file: /cvs/gnome/ORBit2/linc2/include/linc/linc.h,v
retrieving revision 1.17
diff -u -p -r1.17 linc.h
--- linc2/include/linc/linc.h	28 Jan 2005 12:34:57
-0000	1.17
+++ linc2/include/linc/linc.h	4 Dec 2006 13:20:49 -0000
 -33,7
+33,7  GMainLoop *link_main_get_loop    (void);
 guint      link_main_idle_add    (GSourceFunc function,
 				  gpointer    data);

-void       link_wait             (void);
+gboolean   link_wait             (void);
 void       link_signal           (void);

 gboolean   link_thread_io        (void);
Index: linc2/src/linc-connection.c
============================================================
=======
RCS file: /cvs/gnome/ORBit2/linc2/src/linc-connection.c,v
retrieving revision 1.117
diff -u -p -r1.117 linc-connection.c
--- linc2/src/linc-connection.c	9 Sep 2006 07:54:37
-0000	1.117
+++ linc2/src/linc-connection.c	4 Dec 2006 13:20:49 -0000
 -635,8
+635,10  link_connection_do_initiate (LinkConnect
 static LinkConnectionStatus
 link_connection_wait_connected_T (LinkConnection *cnx)
 {
-	while (cnx && cnx->status == LINK_CONNECTING)
-		link_wait ();
+	while (cnx && cnx->status == LINK_CONNECTING) {
+		if (!link_wait ())
+			link_connection_disconnect (cnx);
+	}

 	return cnx ? cnx->status : LINK_DISCONNECTED;
 }
 -659,8
+661,12  link_connection_try_reconnect (LinkConne
 			cnx->inhibit_reconnect = FALSE;
 			dispatch_callbacks_drop_lock (cnx);
 			g_main_context_release (NULL);
-		} else 
-			link_wait ();
+		} else {
+			if (!link_wait ()) {
+				link_connection_disconnect (cnx);
+				break;
+			}
+		}
 	}

 	if (cnx->status != LINK_DISCONNECTED)
Index: linc2/src/linc.c
============================================================
=======
RCS file: /cvs/gnome/ORBit2/linc2/src/linc.c,v
retrieving revision 1.63
diff -u -p -r1.63 linc.c
--- linc2/src/linc.c	2 Jun 2006 08:14:22 -0000	1.63
+++ linc2/src/linc.c	4 Dec 2006 13:20:49 -0000
 -52,6
+52,9  SSL_METHOD *link_ssl_method;
 SSL_CTX    *link_ssl_ctx;
 #endif

+/* max time to wait for the link condition to get signaled
- 10 seconds */
+#define LINK_WAIT_TIMEOUT_USEC (10000000) 
+
 static void link_dispatch_command (gpointer data, gboolean
immediate);

 gboolean
 -534,17
+537,28  link_signal (void)
 	}
 }

-void
+gboolean
 link_wait (void)
 {
+	GTimeVal gtime;
+
 	if (!(link_is_thread_safe &&
link_is_io_in_thread)) {
 		link_unlock ();
 		link_main_iteration (TRUE);
 		link_lock ();
 	} else {
 		g_assert (link_main_cond != NULL);
-		g_cond_wait (link_main_cond, link_main_lock);
+
+		g_get_current_time (&gtime);
+		g_time_val_add (&gtime, LINK_WAIT_TIMEOUT_USEC);
+		if (!g_cond_timed_wait (link_main_cond, link_main_lock,
&gtime)) {
+			if (link_is_locked ())
+				link_unlock ();
+			return FALSE;
+		}
 	}
+
+	return TRUE;
 }

Index: src/orb/GIOP/giop-recv-buffer.c
============================================================
=======
RCS file:
/cvs/gnome/ORBit2/src/orb/GIOP/giop-recv-buffer.c,v
retrieving revision 1.81
diff -u -p -r1.81 giop-recv-buffer.c
--- src/orb/GIOP/giop-recv-buffer.c	5 Apr 2006 07:17:17
-0000	1.81
+++ src/orb/GIOP/giop-recv-buffer.c	4 Dec 2006 13:20:50
-0000
 -690,10
+690,21  check_got (GIOPMessageQueueEntry *ent)
 		(ent->cnx->parent.status == LINK_DISCONNECTED));
 }

+static glong giop_initial_timeout_limit =
GIOP_INITIAL_TIMEOUT_LIMIT;
+
+void
+giop_recv_set_timeout (const glong timeout)
+{
+	if (0 < timeout) /* We really do not want (timeout
<= 0) as that would potentially block forever */
+		giop_initial_timeout_limit = timeout;
+}
+
 GIOPRecvBuffer *
-giop_recv_buffer_get (GIOPMessageQueueEntry *ent)
+giop_recv_buffer_get (GIOPMessageQueueEntry *ent,
+		      gboolean *timeout)
 {
 	GIOPThread *tdata = giop_thread_self ();
+	GTimeVal tval;

  thread_switch:
 	if (giop_thread_io ()) {
 -704,8
+715,17  giop_recv_buffer_get (GIOPMessageQueueEn
 				ent_unlock (ent);
 				giop_thread_queue_process (tdata);
 				ent_lock (ent);
-			} else
-				g_cond_wait (tdata->incoming, tdata->lock);
+			} else {
+				if (0 < giop_initial_timeout_limit) {
+					g_get_current_time (&tval);
+					g_time_val_add (&tval,
giop_initial_timeout_limit);
+				}
+				if (!g_cond_timed_wait (tdata->incoming,
tdata->lock, ((0 < giop_initial_timeout_limit) ?
&tval : NULL))) {
+					*timeout = TRUE;
+					break;
+				} else
+					*timeout = FALSE;
+			}
 		}

 		ent_unlock (ent);
 -1352,3
+1372,4  giop_recv_buffer_use_buf (GIOPConnection

 	return buf;
 }
+
Index: src/orb/orb-core/corba-orb.c
============================================================
=======
RCS file: /cvs/gnome/ORBit2/src/orb/orb-core/corba-orb.c,v
retrieving revision 1.123
diff -u -p -r1.123 corba-orb.c
--- src/orb/orb-core/corba-orb.c	16 Aug 2006 09:28:30
-0000	1.123
+++ src/orb/orb-core/corba-orb.c	4 Dec 2006 13:20:51 -0000
 -61,7
+61,7  static char        *orbit_debug_options 
 static char        *orbit_naming_ref         = NULL;
 static GSList      *orbit_initref_list       = NULL; 
 static gboolean     orbit_use_corbaloc       = FALSE;
-
+static gint         orbit_timeout_limit      = -1;
 void
 ORBit_ORB_start_servers (CORBA_ORB orb)
 {
 -417,8
+417,8  CORBA_ORB_init (int *argc, char **argv,
 	}
 #endif /* G_ENABLE_DEBUG */

-
 	giop_recv_set_limit (orbit_initial_recv_limit);
+	giop_recv_set_timeout (orbit_timeout_limit);
 	giop_init (thread_safe,
 		   orbit_use_ipv4 || orbit_use_ipv6 ||
 		   orbit_use_irda || orbit_use_ssl);
 -1467,6
+1467,7  const ORBit_option orbit_supported_optio
 	{ "ORBDebugFlags",      ORBIT_OPTION_STRING, 
&orbit_debug_options },
 	{ "ORBInitRef",         ORBIT_OPTION_KEY_VALUE, 
&orbit_initref_list},
 	{ "ORBCorbaloc",        ORBIT_OPTION_BOOLEAN,
&orbit_use_corbaloc},
+	{ "GIOPTimeoutLimit",   ORBIT_OPTION_INT,    
&orbit_timeout_limit },
 	{ NULL,                 0,                    NULL },
 };

Index: src/orb/orb-core/orbit-small.c
============================================================
=======
RCS file: /cvs/gnome/ORBit2/src/orb/orb-core/orbit-small.c,v
retrieving revision 1.97
diff -u -p -r1.97 orbit-small.c
--- src/orb/orb-core/orbit-small.c	18 May 2006 14:20:49
-0000	1.97
+++ src/orb/orb-core/orbit-small.c	4 Dec 2006 13:20:51 -0000
 -591,6
+591,7  ORBit_small_invoke_stub (CORBA_Object   
 	GIOPRecvBuffer         *recv_buffer = NULL;
 	CORBA_Object            xt_proxy = CORBA_OBJECT_NIL;
 	ORBitPolicy            *invoke_policy = CORBA_OBJECT_NIL;
+	gboolean                timeout = FALSE;

 	if (!obj) {
 		dprintf (MESSAGES, "Cannot invoke method on null
objectn");
 -654,7
+655,9  ORBit_small_invoke_stub (CORBA_Object   
 		goto clean_out;
 	}

-	recv_buffer = giop_recv_buffer_get (&mqe);
+	recv_buffer = giop_recv_buffer_get (&mqe,
&timeout);
+	if (timeout)
+		goto timeout_exception;

 	switch (orbit_small_demarshal (obj, &cnx, recv_buffer,
ev,
 				       ret, m_data, args))
 -698,6
+701,12  ORBit_small_invoke_stub (CORBA_Object   
 	tprintf ("[System exception comm failure] )");
 	CORBA_exception_set_system (ev, ex_CORBA_COMM_FAILURE,
 				    completion_status);
+	goto clean_out;
+
+ timeout_exception:
+	tprintf ("[System exception timeout] )");
+	CORBA_exception_set_system (ev, ex_CORBA_TIMEOUT,
+				    CORBA_COMPLETED_NO);
 	goto clean_out;
 }

_______________________________________________
orbit-list mailing list
orbit-listgnome.org
htt
p://mail.gnome.org/mailman/listinfo/orbit-list

Timeout support - Please review

Thread: Timeout support - Please review