Pool contention enhancements and bug fixes

NinoFloris · YohDeadfall · commit 6969fd9e6472 · 2019-08-28T23:38:37.000+03:00
Fixes #2513
diff --git a/src/Npgsql/ConnectorPool.cs b/src/Npgsql/ConnectorPool.cs
diff --git a/src/Npgsql/NpgsqlConnection.cs b/src/Npgsql/NpgsqlConnection.cs
@@ -608,6 +608,10 @@ internal void Close(bool wasBroken)
 
             Connector.CloseOngoingOperations();
 
+            // The connector has closed us during CloseOngoingOperations due to an underlying failure.
+            if (Connector == null)
+                return;
+
             if (Settings.Pooling)
             {
                 if (EnlistedTransaction == null)
diff --git a/src/Npgsql/NpgsqlConnector.cs b/src/Npgsql/NpgsqlConnector.cs
@@ -194,6 +194,11 @@ int ReceiveTimeout
         /// </summary>
         internal DateTime ReleaseTimestamp { get; set; } = DateTime.MaxValue;
 
+        /// <summary>
+        /// If pooled, the pool index on which this connector will be returned to the pool.
+        /// </summary>
+        internal int PoolIndex { get; set; } = int.MaxValue;
+
         internal int ClearCounter { get; set; }
 
         static readonly NpgsqlLogger Log = NpgsqlLogManager.CreateLogger(nameof(NpgsqlConnector));
diff --git a/src/Npgsql/NpgsqlEventSource.cs b/src/Npgsql/NpgsqlEventSource.cs
@@ -98,7 +98,8 @@ static int GetBusyConnections()
                 var pool = kv.Pool;
                 if (pool == null)
                     return sum;
-                sum += pool.State.Busy;
+                var (_, _, busy) = pool.State;
+                sum += busy;
             }
             return sum;
         }
diff --git a/test/Npgsql.Tests/PoolManagerTests.cs b/test/Npgsql.Tests/PoolManagerTests.cs
@@ -47,7 +47,7 @@ public void ClearAll()
 
             NpgsqlConnection.ClearAllPools();
             Assert.That(pool.State.Idle, Is.Zero);
-            Assert.That(pool.State.Total, Is.Zero);
+            Assert.That(pool.State.Open, Is.Zero);
         }
 
         [Test]
@@ -62,10 +62,10 @@ public void ClearAllWithBusy()
                 NpgsqlConnection.ClearAllPools();
                 Assert.That(PoolManager.TryGetValue(ConnectionString, out pool), Is.True);
                 Assert.That(pool!.State.Idle, Is.Zero);
-                Assert.That(pool.State.Total, Is.EqualTo(1));
+                Assert.That(pool.State.Open, Is.EqualTo(1));
             }
             Assert.That(pool.State.Idle, Is.Zero);
-            Assert.That(pool.State.Total, Is.Zero);
+            Assert.That(pool.State.Open, Is.Zero);
         }
 
         [SetUp]
diff --git a/test/Npgsql.Tests/PoolTests.cs b/test/Npgsql.Tests/PoolTests.cs
@@ -14,7 +14,7 @@ class PoolTests : TestBase
         [Test]
         public void MinPoolSizeEqualsMaxPoolSize()
         {
-            using (var conn = new NpgsqlConnection(new NpgsqlConnectionStringBuilder(ConnectionString) 
+            using (var conn = new NpgsqlConnection(new NpgsqlConnectionStringBuilder(ConnectionString)
             {
                 MinPoolSize = 30,
                 MaxPoolSize = 30
@@ -59,7 +59,7 @@ public void ReuseConnectorBeforeCreatingNew()
         [Test, Timeout(10000)]
         public void GetConnectorFromExhaustedPool()
         {
-            var connString = new NpgsqlConnectionStringBuilder(ConnectionString) 
+            var connString = new NpgsqlConnectionStringBuilder(ConnectionString)
             {
                 MaxPoolSize = 1,
                 Timeout = 0
@@ -101,7 +101,7 @@ public async Task GetConnectorFromExhaustedPoolAsync()
         [Test]
         public void TimeoutGettingConnectorFromExhaustedPool()
         {
-            var connString = new NpgsqlConnectionStringBuilder(ConnectionString) 
+            var connString = new NpgsqlConnectionStringBuilder(ConnectionString)
             {
                 MaxPoolSize = 1,
                 Timeout = 2
@@ -141,42 +141,11 @@ public async Task TimeoutGettingConnectorFromExhaustedPoolAsync()
                 conn3.Open();
         }
 
-        [Test]
-        public void OverflowExceptionWhenTooManyWaiting()
-        {
-            var connString = new NpgsqlConnectionStringBuilder(ConnectionString) 
-            {
-                ApplicationName = nameof(OverflowExceptionWhenTooManyWaiting),
-                MaxPoolSize = 1,
-            }.ToString();
-
-            using (var conn = new NpgsqlConnection(connString))
-            {
-                conn.Open();
-                Assert.True(PoolManager.TryGetValue(connString, out var pool));
-                var state = pool!.State;
-
-                try
-                {
-                    var newState = state;
-                    newState.Waiting = int.MaxValue;
-                    pool!.State = newState;
-                    var conn2 = new NpgsqlConnection(connString);
-                    Assert.Catch<OverflowException>(() => conn2.Open());
-                }
-                finally
-                {
-                    // Restore state for the closes work correctly.
-                    pool!.State = state;
-                }
-            }
-        }
-
         //[Test, Timeout(10000)]
         //[Explicit("Timing-based")]
         public async Task CancelOpenAsync()
         {
-            var connString = new NpgsqlConnectionStringBuilder(ConnectionString) 
+            var connString = new NpgsqlConnectionStringBuilder(ConnectionString)
             {
                 ApplicationName = nameof(CancelOpenAsync),
                 MaxPoolSize = 1,
@@ -338,7 +307,7 @@ public void ClearWithBusy()
         [Test]
         public void ClearWithNoPool()
         {
-            var connString = new NpgsqlConnectionStringBuilder(ConnectionString) 
+            var connString = new NpgsqlConnectionStringBuilder(ConnectionString)
             {
                 ApplicationName = nameof(ClearWithNoPool)
             }.ToString();
@@ -349,7 +318,7 @@ public void ClearWithNoPool()
         [Test, Description("https://github.com/npgsql/npgsql/commit/45e33ecef21f75f51a625c7b919a50da3ed8e920#r28239653")]
         public void PhysicalOpenFailure()
         {
-            var connString = new NpgsqlConnectionStringBuilder(ConnectionString) 
+            var connString = new NpgsqlConnectionStringBuilder(ConnectionString)
             {
                 ApplicationName = nameof(PhysicalOpenFailure),
                 Port = 44444,
@@ -373,7 +342,7 @@ public void PhysicalOpenFailure()
         //[TestCase(10, 20, 30, false)]
         public void ExercisePool(int maxPoolSize, int numTasks, int seconds, bool async)
         {
-            var connString = new NpgsqlConnectionStringBuilder(ConnectionString) 
+            var connString = new NpgsqlConnectionStringBuilder(ConnectionString)
             {
                 ApplicationName = nameof(ExercisePool),
                 MaxPoolSize = maxPoolSize
@@ -409,8 +378,8 @@ void AssertPoolState(ConnectorPool? pool, int idle, int busy, int waiting=0)
 
             var state = pool.State;
             Assert.That(state.Idle, Is.EqualTo(idle), $"Idle should be {idle} but is {state.Idle}");
-            Assert.That(state.Busy, Is.EqualTo(busy), $"Busy should be {busy} but is {state.Busy}");
-            Assert.That(state.Waiting, Is.EqualTo(waiting), $"Waiting should be {waiting} but is {state.Waiting}");
+            var stateBusy = state.Open - state.Idle;
+            Assert.That(stateBusy, Is.EqualTo(busy), $"Busy should be {busy} but is {stateBusy}");
         }
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -608,6 +608,10 @@ internal void Close(bool wasBroken)`
`608`	`608`
`609`	`609`	`Connector.CloseOngoingOperations();`
`610`	`610`
	`611`	`+ // The connector has closed us during CloseOngoingOperations due to an underlying failure.`
	`612`	`+ if (Connector == null)`
	`613`	`+ return;`
	`614`	`+`
`611`	`615`	`if (Settings.Pooling)`
`612`	`616`	`{`
`613`	`617`	`if (EnlistedTransaction == null)`
Original file line number	Diff line number	Diff line change
`@@ -98,7 +98,8 @@ static int GetBusyConnections()`
`98`	`98`	`var pool = kv.Pool;`
`99`	`99`	`if (pool == null)`
`100`	`100`	`return sum;`
`101`		`- sum += pool.State.Busy;`
	`101`	`+ var (_, _, busy) = pool.State;`
	`102`	`+ sum += busy;`
`102`	`103`	`}`
`103`	`104`	`return sum;`
`104`	`105`	`}`
Original file line number	Diff line number	Diff line change
`@@ -47,7 +47,7 @@ public void ClearAll()`
`47`	`47`
`48`	`48`	`NpgsqlConnection.ClearAllPools();`
`49`	`49`	`Assert.That(pool.State.Idle, Is.Zero);`
`50`		`- Assert.That(pool.State.Total, Is.Zero);`
	`50`	`+ Assert.That(pool.State.Open, Is.Zero);`
`51`	`51`	`}`
`52`	`52`
`53`	`53`	`[Test]`
`@@ -62,10 +62,10 @@ public void ClearAllWithBusy()`
`62`	`62`	`NpgsqlConnection.ClearAllPools();`
`63`	`63`	`Assert.That(PoolManager.TryGetValue(ConnectionString, out pool), Is.True);`
`64`	`64`	`Assert.That(pool!.State.Idle, Is.Zero);`
`65`		`- Assert.That(pool.State.Total, Is.EqualTo(1));`
	`65`	`+ Assert.That(pool.State.Open, Is.EqualTo(1));`
`66`	`66`	`}`
`67`	`67`	`Assert.That(pool.State.Idle, Is.Zero);`
`68`		`- Assert.That(pool.State.Total, Is.Zero);`
	`68`	`+ Assert.That(pool.State.Open, Is.Zero);`
`69`	`69`	`}`
`70`	`70`
`71`	`71`	`[SetUp]`