[Mapserver-dev] Re: mappool.c thoughts

[Sean Gillies]

Thanks for forwarding, more below ...

On Oct 5, 2004, at 9:14 AM, Frank Warmerdam wrote:

> Sean Gillies wrote:
>> Hi Frank,
>> I've finally made the time to read through mappool.c.  It's
>> clearly a huge improvement over the old "same connection as"
>> sharing, thanks!  I'd like to use it in my mod_python applications
>> (which have a lot in common with fastcgi) and also for my
>> mapserver/Zope project (if possible).
> > What would you think about making it more object oriented?  I
> > think that having a module-level pool will be problematic for
> > Python threads, and that it could be better to make the pool
> > a class and let user/programmers deal with how to make access thread
> > safe.
> >
> > Right now the pool is implemented as 3 module-level variables,
> > could we instead have a ConnectionPool like
> >
> >   typedef struct {
> >       int count;
> >       int maxcount;
> >       connectionObj *connections;
> >   } ConnectionPool;
> >
>> ?  I imagine that the mapserv app object (a mapservObj) would
>> contain an instance of this.  Likewise, mapscript applications
>> could create an instance and use it like
>>   pool = mapscript.ConnectionPool()
>>   for layer in postgis_layers:
>>         pool.register(layer, ...)
>> The connection pool would be thread local by default. If someone
>> wanted to program a multi-threaded app, they could deal with
>> threading and locking themselves.
> >
>> This complicates the implementation.  Would require adding a
>> connectionObj * argument to every one of the msConnPool functions,
>> and we'd also have to pass a pointer into msLoadMap so that layer
>> connnections are registered when loaded.  Do you see any other
>> complications?
>
> Sean,
>
> I think what I am least clear on is how we know what connection
> pool to use down at the layer level.  A function like 
> msPOSTGISLayerOpen()
> takes in *only* a layerObj.  It doesn't know what map that layer is
> associated with.  It doesn't know what mapservObj (if any) is being 
> used
> to process it.
>
> I don't mind adding a ConnectionPool * to the functions.  What I think 
> is
> much less paletable is having to pass connection pools up and down the
> how tree of functions.  (msDrawMap() / msLoadMap() / msOpenLayer() /
> msPOSTGISOpenLayer()) and so on.
>
> In fact, this is the same sort of problem we face with generalizing he
> IO redirection.  Low level MapServer functions generally get a very 
> limited
> amount of context information to execute their operations.  For 
> instance,
> msSetError() doesn't have any idea of it's external context though it 
> assumes
> it can safely managed its own internal per-thread stack of error 
> objects.
>
> If our objective with connection pools is to make them thread-local or 
> to
> allow that degree of control, why not just implement that in the 
> mappool API
> possibly along with some "policy" control functions?
>
> It seems to me that unless we are going to substantially most 
> functions in
> MapServer, we are going to need an easy way of managing thread local 
> "globals"
> that doesn't involve passing thread context around everywhere.  Pools 
> are one
> example.
>
> --
>
> Setting aside that broader issue,  I would be willing to implement the
> suggested ConnectionPool structure.  The various connection pool 
> functions
> would have a ConnectionPool object added as their first argument.
>
> My caveats are this:
>
>  o It would always be legal to pass NULL as the ConnectionPool which 
> would cause
>    mappool.c to select an appropriate pool.
>
>  o By default a global pool would be used (managed internally as a 
> static).
>
>  o If the layerObj has a "CONNECTION_POOL=%p" directive, the pointer 
> value from
>    that would be used instead.   This is how python mapScript would 
> have to
>    force a particular layer to use a particular pool.
>
>  o msConnPoolRegister() would have an extra argument indicating if a 
> given
>    connection is "this thread only", "one thread at a time" or 
> "multithread safe".
>    The connection pool would track that status, and also (when the 
> ref_count
>    is non-zero) what thread is using it if it is not "multithread 
> safe".
>
>  o I would also ensure that any use of globals in mappool is protected 
> by a
>    mutex.
>
> Note, the above changes do not require any api changes higher than the
> msConnPool API.  Furthermore, I think it provides good default 
> multi-thread
> safety and some ability to tinker for those who want more control over 
> the
> locking themselves.
>
> BTW, when registering connections (in advance) from MapScript I am 
> assuming
> a NULL close function would be provided and the mapscript app would 
> take care
> of connection cleanup.  Of course, a wrapper close() callback could 
> also be
> provided.
>
> Finally, I took the liberty of cc:ing this back to the -dev list 
> because I
> think some of the issues are of broader interest and I want to get a 
> feel for
> where the rest of the developers stand with regard to general thread 
> safety
> issues.
>

Frank,

I think it's a good thing that our functions require just the bare 
minimum
context.  Passing a connection pool everywhere would suck.  I'm happy 
with
your proposed solution, with one additional suggestion.

Let's consider adding a

   ConnectionPool *connectionpool

member to layerObj.  If NULL, we use the default module-level pool.  In 
the
mapscript case, it would reference an application-level pool.  Calls to
the msConnPool functions could be passed self->connectionpool.

Sean

--
Sean Gillies
sgillies at frii dot com
http://users.frii.com/sgillies