Skip to content

Clear all pools also deletes pools #3395

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
vonzshik merged 5 commits into from
Dec 11, 2020
Merged

Clear all pools also deletes pools #3395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c5b9ff6
ClearAllPools now also removes all pools
vonzshik Dec 7, 2020
89c2b7e
Fixed test
vonzshik Dec 7, 2020
4b37845
Added a test for a deleted pool
vonzshik Dec 7, 2020
1b9f9d9
ClearAllPools now also always deletes pools
vonzshik Dec 11, 2020
67f67f5
Fixed a race condition
vonzshik Dec 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 16 additions & 18 deletions src/Npgsql/PoolManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,21 @@ internal static bool TryGetValue(string key, [NotNullWhen(true)] out ConnectorPo
// Note that pools never get removed. _pools is strictly append-only.
var nextSlot = _nextSlot;
var pools = _pools;
// There is a race condition between TryGetValue and ClearAll.
// When the nextSlot has already been read as a non-zero value, but the pools have already been reset.
// And in this case, we will iterate beyond the size of the array.
// As a safeguard, we take a min length between the array and the nextSlot.
var minNextSlot = Math.Min(nextSlot, pools.Length);
var sw = new SpinWait();

// First scan the pools and do reference equality on the connection strings
for (var i = 0; i < nextSlot; i++)
for (var i = 0; i < minNextSlot; i++)
{
var cp = pools[i];
if (ReferenceEquals(cp.Key, key))
{
// It's possible that this pool entry is currently being written: the connection string
// component has already been writte, but the pool component is just about to be. So we
// component has already been written, but the pool component is just about to be. So we
// loop on the pool until it's non-null
while (Volatile.Read(ref cp.Pool) == null)
sw.SpinOnce();
Expand All @@ -46,7 +51,7 @@ internal static bool TryGetValue(string key, [NotNullWhen(true)] out ConnectorPo
}

// Next try value comparison on the strings
for (var i = 0; i < nextSlot; i++)
for (var i = 0; i < minNextSlot; i++)
{
var cp = pools[i];
if (cp.Key == key)
Expand Down Expand Up @@ -99,10 +104,17 @@ internal static void ClearAll()
for (var i = 0; i < _nextSlot; i++)
{
var cp = pools[i];
// We shouldn't ever get here, as _nextSlot is only incremented under a lock
// and there shouldn't be null values in-between. But just in case...
if (cp.Key == null)
return;
break;
cp.Pool?.Clear();
}

// _nextSlot should be changed before the _pools.
// Otherwise, there will be a race condition with TryGetValue.
_nextSlot = 0;
_pools = new (string, ConnectorPool)[InitialPoolsSize];
}
}

Expand All @@ -113,19 +125,5 @@ static PoolManager()
AppDomain.CurrentDomain.DomainUnload += (sender, args) => ClearAll();
AppDomain.CurrentDomain.ProcessExit += (sender, args) => ClearAll();
}

/// <summary>
/// Resets the pool manager to its initial state, for test purposes only.
/// Assumes that no other threads are accessing the pool.
/// </summary>
internal static void Reset()
{
lock (Lock)
{
ClearAll();
_pools = new (string, ConnectorPool)[InitialPoolsSize];
_nextSlot = 0;
}
}
}
}
15 changes: 5 additions & 10 deletions test/Npgsql.Tests/PoolManagerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public void WithCanonicalConnString()
[Test]
public void ManyPools()
{
PoolManager.Reset();
PoolManager.ClearAll();
for (var i = 0; i < PoolManager.InitialPoolsSize + 1; i++)
{
var connString = new NpgsqlConnectionStringBuilder(ConnectionString)
Expand All @@ -33,7 +33,7 @@ public void ManyPools()
using (var conn = new NpgsqlConnection(connString))
conn.Open();
}
PoolManager.Reset();
PoolManager.ClearAll();
}
#endif

Expand All @@ -53,25 +53,20 @@ public void ClearAll()
[Test]
public void ClearAllWithBusy()
{
ConnectorPool? pool;
using (OpenConnection())
{
using (OpenConnection()) { }
// We have one idle, one busy

NpgsqlConnection.ClearAllPools();
Assert.That(PoolManager.TryGetValue(ConnectionString, out pool), Is.True);
Assert.That(pool!.Statistics.Idle, Is.Zero);
Assert.That(pool.Statistics.Total, Is.EqualTo(1));
Assert.That(PoolManager.TryGetValue(ConnectionString, out var pool), Is.False);
}
Assert.That(pool.Statistics.Idle, Is.Zero);
Assert.That(pool.Statistics.Total, Is.Zero);
}

[SetUp]
public void Setup() => PoolManager.Reset();
public void Setup() => PoolManager.ClearAll();

[TearDown]
public void Teardown() => PoolManager.Reset();
public void Teardown() => PoolManager.ClearAll();
}
}