[Openais] Re: just looked at some of the openais code, have some comments

[Steven Dake]

Responses inline

On Thu, 2004-06-24 at 14:29, Chris Friesen wrote:
> I just took a bit of a walk through the clm test code and traced it a bit 
> through the library code.  Here are my thoughts:
> 
> 
> --I don't think it makes sense to use mutexes unconditionally.  For 
> single-threaded apps its only extra overhead.  They should be conditionally 
> compiled based on compiler flag (REENTRANT or similar).
> 

In GCC, if you link with -lpthread, mutexes are used.  If you dont link
with -lpthread, they are not used.  This is probably just a feature of
gcc, which hurts portablilty somewhat.

> --It violates design principles to lock/unlock mutexes in different methods, but 
> here it is even in different conceptual levels of code. Specifically, 
> saHandleConvert() passing back with mutex locked, then unlocking it in the caller.
> 

I am not pleased with the locking approach at the moment.  The reason
the mutex must be unlocked in the caller of saHandleConvert is to
protect against races in Initialize/Finalize.  If locking were in the
actual function, an unfree could make the handle returned back from
HandleConvert invalid.  Another important reason is that data in the
instance must be accessed while the lock is held.

On the plus side, all of the complexities of managing the handle
database and associated locks are mostly removed from the library
implementors except for ensuring that an unlock takes place after a
HandleConvert.

I do take patches :)

> --The unprotected (and unconditional) mutex use means that you can't use any of 
> the library functions from a signal handler.  I didn't see this documented in 
> any of the headers.

I intend to create a FAQ and I'll add this to that document.

You are correct.  One thing I would wonder though, is could an API user
really use a signal handler at all with the APIs?  If the handle
database is in mid-update, and then a signal occurs, the API calls may
not work appropriately 100% of the time.  Any ideas of supporting this
sort of feature?
  
> 
> --When calling saClmDispatch, we may already know (as is the case in the test 
> code) that there is at least one message.  In that case we could skip the 
> saPollRetry()/saHandleConvert() calls and go directly to saRecvRetry().
> 

How do you always know there is atleast one message?

> --In saClmDispatch, do you really want to drop the instance mutex before calling 
> the callbacks?  What happens if another thread removes the instance and then you 
> call the callback function which is now invalid?  If you do want to drop the 
> mutex, maybe the instance should be refcounted or something?
> 

This is a bug and has been fixed in the latest bk by copying the data
handed back in the message to a storage buffer, then unlocking.  Not
pretty, but it works.

If the lock is not dropped before the callback, other threads could
deadlock accessing other API functions from callbacks.

> --The testclm.c demo doesn't have any way to shut down cleanly as far as I can 
> tell.  If you kill it, then saClmClusterTrackStop() and saClmFinalize() never 
> get called.  I guess that demos the library cleanup code....
> 

The AIS executive fully handles API clients that exit unexpectedly.  The
demo program is far from perfect..  If you have some good examples, it
can be included..

> --Do you really want to kill the executive rather than drop messages if you run 
> out of buffers?
> 

No absolutely not.

There are a few exit() calls in the code that should be removed and
better overload handling added.  Rather then take a half-hearted stab, I
left the exit's in so people could see exactly where the problems are.

> 
> Anyone have any comments?  I'm not subscribed to the list, so please cc me on 
> responses.
> 

Thanks for taking the time to look
-steve

> Thanks,
> 
> Chris
> 
> 
> 
>