SCM

[#1011346] Thread synchronization inside GetPooledConnector

View Trackers | Bugs | Download .csv | Monitor

Date:
2013-07-18 10:31
Priority:
3
State:
Closed
Submitted by:
Ilya G (beast)
Assigned to:
Nobody (None)
Npgsql Version:
2.0.11
Category:
None
Group:
None
Resolution:
Accepted
Summary:
Thread synchronization inside GetPooledConnector

Detailed description
I have found really nasty bug with locks, when Npgsql works in pooling mode, inside functions GetPooledConnector and UngetPooledConnector.

The symptoms were a lot of opening physical connections (more than I set in MaxPoolSize variable). So I tried figure out why this happens.

My program has a lot of threads which use Npgsql for queries that's why I'm using Npgsql in pooling mode.
After some research I understood that next code opens new physical connection, when we are not reach MaxPoolSize limit yet:
[code]
lock (Queue)
{
if (Queue.Available.Count + Queue.Busy.Count < Connection.MaxPoolSize)
{
Connector = CreateConnector(Connection);
Queue.Busy.Add(Connector, null);
}
}
[/code]
(it's from GetPooledConnector function)

But the counter Queue.Available.Count and Queue.Busy.Count decrements and increments not "at once" inside UngetPooledConnector function. Look at its code:
[code]
...
lock (queue)
{
inQueue = queue.Busy.ContainsKey(Connector);
queue.Busy.Remove(Connector);
}

...some code, and then...

lock (queue)
{
...
if (inQueue)
queue.Available.Enqueue(Connector);
...
}
[/code]
As you can see in the first part of UngetPooledConnector we lock "Queue" and remove our busy connector, then unlock "Queue", then doing some actions, then lock "Queue" again and adding connector to the Available list.

So it possible that
- one thread which has called UngetPooledConnector and Remove own Connector from busy list already unlocked Queue, but still not added Connector to Available list
- and in the same time, another thread which called GetPooledConnector check condition of
"if (Queue.Available.Count + Queue.Busy.Count < Connection.MaxPoolSize)" and it will be positive because we are not added removed connector to Available list!


My fix is change UngetPooledConnector like this:

[code]
lock (queue)
{
inQueue = queue.Busy.ContainsKey(Connector);
queue.Busy.Remove(Connector);

...

if (inQueue)
queue.Available.Enqueue(Connector);
else
Connector.Close();
...
}
[/code]
Now it will be in same time "Busy.Remove" and "Available.Enqueue" calls. But I'm not sure that it's best fix. =)

Also, as I can see the bug was introduced in 2.0.11.91 version (because you had removed "Queue.UseCount" counter) and still exist through beta version Npgsql2 2.0.13 beta1

Followup

Message
Date: 2013-09-24 17:37
Sender: Francisco Figueiredo jr.

Fixed in git. Checkout this pull request:
https://github.com/franciscojunior/Npgsql2/pull/33
Date: 2013-07-19 19:17
Sender: Francisco Figueiredo jr.

Nice catch!

I created a patch to fix this error: https://github.com/franciscojunior/Npgsql2/commit/8e05232bb780a811c1d9184fa2261f2a6ea9a352

Now the remove and add are done inside the same lock.

Please, give it a try and let me know if it works for you.

Note that this fix isn't in the master branch yet. You can get the complete code with this fix in the following bugfix branch: https://github.com/franciscojunior/Npgsql2/tree/bug-1011346-thread-synchronization

Attached Files:

Changes:

Field Old Value Date By
ResolutionNone2013-09-24 17:37fxjr
status_idOpen2013-09-24 17:37fxjr
close_dateNone2013-09-24 17:37fxjr
Powered By FusionForge