[PATCH] 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  <colding omesc com>
+
+	* configure.in: Added autoconf 2.60+ required datarootdir variable
+
+2006-12-03  Jules Colding  <colding omesc 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  <kmaraas gnome 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  <colding omesc com>
+
+	* src/linc.c (link_wait): Do not wait forever for the link
+	condition to get signaled.
+
 2006-11-22  Kjartan Maraas  <kmaraas gnome 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 object\n");
@@ -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;
 }
 




[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]