Client hangs/crashes when server goes down unexpectedly

I have had some problems with my BEEP client hanging or crashing in certain
exceptional circumstances, e.g. if the server it is talking to is terminated
in mid-conversation.  I have seen the same behaviour in the test program,
beepng.  
(I am using beepcore v0.2 with patches)

There are three main places where I have seen it hang:

1.	if beepng doesn't get a start confirmation (e.g. beepd was
terminated before it sent the confirmation).  bp_start_request hangs waiting
for user_sem1. (bp_wrapper.c line 564 )
2.	if beepng doesn't get a reply to a  message - null_trip hangs
waiting for pl_sem to be released.  (null-profiles.c line 689)
3.	if beepng doesn't get a close confirmation - null_close hangs
waiting for PRO_CLSWAIT flag in pl_flags to be unset.  (null-profiles.c line
738)

I have also seen beepng hang after being run many times in a row (e.g.
(arbitrary) 70 times).  Not sure why this situation comes about.

In the case that the server is terminated midconversation and the socket is
closed unexpectedly, presumably some cleanup of the semaphores/flags is
missing.  It looks like session_shutdown or something similar is intended to
be called in shutdown_callback.  This would probably look after cleaning up
user_sem1 and call pro_close_confirmation to clean up the other stuff.
However, it is #ifdef'ed out at the moment. (shutdown_callback seems to get
called indirectly from checkdown when IW_fpollmgr has flagged the socket
error).

What if the server just doesn't reply, e.g. due to programming error, etc?
This would also cause beepng to hang forever.  Where the client is part of a
larger system or long-running program, this can be a problem.  Should there
not be some kind of timeout?  Is this intentional in the BEEP protocol or
just an implementation issue?


In order to get around this, I have implemented simple timeouts - example
below if it is of use to anyone else. 

For 1. above - in bp_start_request (in threaded_os/wrapper/bp_wrapper.c):


465a466,469
>     /* hryan 23 Dec 02: needed for change below */
>     int i;
>     int sem_acquired = 0;
>     int sleeptime    = 0;
564c568,600
<         SEM_WAIT(&inst->user_sem1);
---
>
>         /* SEM_WAIT(&inst->user_sem1); */
>         /* hryan 23 Dec 02: rather than block, try a few times to get
>            semaphore, since sometimes it hangs */
>         /* hryan TODO: num retries/sleep time should be configurable */
>         sem_acquired = 0;
>         sleeptime    = 0;   /* don't sleep at first */
>         for (i = 0; i < 50 && sem_acquired == 0; i++)
>         {
>             if (i > 20)   /* after a few times, ease off a bit */
>             {
>                 sleeptime = 1;
>             }
>             if (sleeptime > 0)
>             {
>                  sleep(sleeptime);
>             }
>             if (SEM_TRYWAIT(&inst->user_sem1) == 0)
>             {
>                 sem_acquired = 1;
>             }
>         }
>         if (sem_acquired != 1)
>         {
>             /* We have not been able to create the channel, so
>                release the semaphore (user_sem1 seems to be locked
>                while the channel is in existence) */
>             SEM_POST(&inst->user_sem1);
>             return(blw_diagnostic_new(wrap, BLW_BEEP_LIBRARY_ERROR, NULL,
>                                       "could not get semaphore"));
>
>         }
>         /* end hryan 23 Dec 02 change */


For 2. and 3., I do similar loops at the points where null_trip is waiting
for pl_sem, and where null_close is waiting for PRO_CLSWAIT to be unset.  If
no sign of the reply/close confirmation, then

        SEM_DESTROY (&il -> pl_sem);
        lib_free (il);
        return NULL_DONE;

These changes seem to occasionally cause beepd to get terminated by a
SIGPIPE signal (presumably because the socket gets closed unexpectedly by
the client).

(Note that you may get an Arithmetic exception if null_trip fails - seems to
just need a 'if (cnt > 0)' before the printf at beepng.c line 656.  Also the
return value from null_close isn't correctly reported because cc doesn't get
updated)



Under similar situations (e.g. where I terminate the server at various
points), beepng will sometimes crash.  This seems to be due to race
conditions between threads?  e.g. where the closing of the socket causes one
thread to set e.g. iostate to null, but the application thread may still try
to access what iostate is pointing to.  Here is a list of places wehre I
found this to cause beepng to crash.  I got around the problem by just
avoiding accessing the pointed-to-structures if the pointer was null -
however  perhaps some cleanup operations get bypassed as a result.

-	In bpc_buffer_allocate: do not use f->payload if it is null (in
./threaded_os/wrapper/bpc_wrapper.c)
-	The iostate member of the wrapper structure: handle_socket_error
seems to cause checkdown to indirectly call fiostate_delete, which frees the
iostate.  However the iostate/'ios' pointer might be later used by
fiostate_unblock_write, fiostate_stop, (both in
./threaded_os/transport/bp_fiostate.c), tcp_bp_connection_close (in
./threaded_os/transport/bp_tcp.c), bp_connection_destroy
(./threaded_os/wrapper/bp_wrapper.c, line 209).  Also it seems that
fiostate_delete (in ./threaded_os/transport/bp_fiostate.c) may sometimes be
called with a null? ios pointer where session_start calls session_shutdown
(towards the end of the session_start function).  So i check for iostate/ios
being null before using it in these functions.

Probably because of the way I time out waiting for responses (see above)
(which may arrive later when they are not expected), I found I needed to
make the following changes also:
-	in notify_lower (in threaded_os/transport/bp_fcbeeptcp.c), in case 8
of the switch(err) statement, do not call blw_put_remembered_error unless
wrap->error_remembered is not null
-	in notify_upper (in threaded_os/wrapper/bp_notify.c), replace the
ASSERT(wrap->iostate != NULL); with an if statement (if (wrap->iostate !=
NULL) ...)


Unfortunately, I am still getting the odd crash when I run beepng many
times, e.g. 

#0  blw_greeting_notification (wrap=0x2ae88, notify=0x0)
    at ..//bp_wrapper.c:390
390         for (pc = 0, p = notify->profile; p != NULL; pc += 1, p =
p->next)
(gdb) where
#0  blw_greeting_notification (wrap=0x2ae88, notify=0x0)
    at ..//bp_wrapper.c:390
#1  0xff033da8 in IW_notify_handle_chan0 (nrvoid=0x2b9d0)
    at ..//bp_notify.c:587
#2  0xff0331d0 in nr_callback (data=0x2b9d0) at ..//bp_notify.c:49
#3  0xff07232c in work_thread (data=0x26650) at ../workthread.c:124
(gdb) 


If anyone has any suggestions on these issues, I would be very grateful if
you could let me know.

Many thanks,
Helen Ryan





-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf