[Mapserver-dev] Re: mappool.c thoughts

[Frank Warmerdam]

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.

Best regards,
-- 
---------------------------------------+--------------------------------------
I set the clouds in motion - turn up   | Frank Warmerdam, warmerdam at pobox.com
light and sound - activate the windows | http://pobox.com/~warmerdam
and watch the world go round - Rush    | Geospatial Programmer for Rent