msConnPoolRelease-bug in multi-threading

[umn-ms@…]

msConnPoolRelease doesn't behave correctly in a multithreading-environment.

Code in msConnPoolRelease:
...
if( layer->connectiontype == conn->connectiontype
    && strcasecmp( layer->connection, conn->connection ) == 0 )
{
  if( conn->conn_handle != conn_handle )
   <kind of assertion>
....
This code expresses: The connections-array may not cotain more 
than one connection with same connection-description (=connectiontype
/connectionSting)

This assumption is not valid in a Multithreading case.
The usage of the threadId in msConnPoolRequest
shows, that this *can* happen. (And really do happen in practice)

The assertion-message raises in multithreading-cases and
connections are not closed.

The problem is not dependend of the use of Connection-Pooling.
It raises in the 'PROCESSING  "CLOSE_CONNECTION=NORMAL"'-case also.

My guess: The two if's should be changed and logically inverted:
...
if( conn->conn_handle == conn_handle )
{
  if( layer->connectiontype != conn->connectiontype
      || strcasecmp( layer->connection, conn->connection ) != 0 )
   <kind of assertion>
....

[umn-ms@…]

Reassigned this bug to frank warmerdam after detecting that
he seems to be the author of mappool.c.
(headerline of mappool.c)

[fwarmerdam]

Dear Hydrotec, 

I see your point!  The problem occurred due to the after-the-fact introduction
of thread-local restrictions on connections.  It seems I missed the release
issue.  I think the more proper solution would be the following change, which
also ensures that the connection being released is held by current thread. 

        if( layer->connectiontype == conn->connectiontype
            && strcasecmp( layer->connection, conn->connection ) == 0 
            && conn->thread_id == msGetThreadId() )
        {

Could you try this out and report back in this bug report if the fix works
properly or not?  If it works then I will incorporate it into 4.7 and the
4.6 stable branch.

[fsimon@…]

Just adding myself. :)

[umn-ms@…]

Hi Frank and Fernando

We can do the test you asked for. But:

"Franks way" to fix the bug will enforce that the connection is released
by the same thread in which it was created.

At least the Java/Mapscript-Binding has the idea, that cleaning up could be
made done by the Java-garbage-collector. (the Java-finalizers calls delete).

In this case the connection-release-thread will be the thread of 
the garbage collector and will therfore not be the connection-creator-thread.

I cannot see the necessity to build in such a "don't pass a open layer to
another thread and close it there"-assumption at this place.

(Never mind. It's *your* code :-) And I also have to confess: 
I would always try to make my programs in a way that 
the long-name-assumption is fulfilled. I just reported bug 1400 in
this context ...)

[fwarmerdam]

Hydrotec, 

The concept of a connection being specific to a single thread while it is in
use was implemented at the request of one of the backends (SDE?).  The jist
of it was that the connection would belong to a single thread till that 
thread released it.  This was to ensure that different threads didn't mess
up "in progress" context on the connection which made sense.  

The expected workflow then is for the same thread to release it when no longer
needed. 

Now this isn't absolutely necessary.  I could indeed allow any thread to
release a connection - and instead of checking thread_id, check connection
handle in the comparison.  But I am concerned that this might also imply that
different threads are using the connection.  If this is occuring, some other
level of per-thread connection lockdown needs to be done to avoid clashes. 

To this end, I have modified the code to look like:

    msAcquireLock( TLOCK_POOL );
    for( i = 0; i < connectionCount; i++ )
    {
        connectionObj *conn = connections + i;

        if( layer->connectiontype == conn->connectiontype
            && strcasecmp( layer->connection, conn->connection ) == 0 
            && conn->conn_handle == conn_handle )
        {
            conn->ref_count--;
            conn->last_used = time(NULL);

            if( conn->ref_count == 0 )
                conn->thread_id = 0;

            if( conn->ref_count == 0 && conn->lifespan == MS_LIFE_ZEROREF )
                msConnPoolClose( i );

            msReleaseLock( TLOCK_POOL );
            return;
        }
    }

Please test out this approach and let me know how it goes here.

Dang, I hate multithreaded programming.  So many subtle things that 
can go wrong. 

[umn-ms@…]

Hi Frank and Fernando

For shure it is a good idea take care, that one connection is used
by only one thread at a time.

We made the test with the second solution:
if( layer->connectiontype == conn->connectiontype
     && strcasecmp( layer->connection, conn->connection ) == 0 
     && conn->conn_handle == conn_handle )
{

It's ok. As far as I'm concerned this bug could be closed now.

Thank you very much for support, Frank!

Benedikt Rothe

PS: And I will *not* complain that "conn->conn_handle == conn_handle"
implies the two other conditions and that these are therfore unnecessary :-)

[fwarmerdam]

Benedict, 

I'm glad you didn't mention the redundancy. :-)

I am taking off for a nearly weeklong long weekend now, but will apply 
the change in 4.6.x and 4.7 when I get back.   Thanks for your help!

[fwarmerdam]

Patch applied in 4.6 and 4.7 (cvshead).