Merge branch 'hotfix/5.0.5' into stable

Directory.Build.props

@@ -1,6 +1,6 @@
 <Project>
   <PropertyGroup>
-    <VersionPrefix>5.0.3</VersionPrefix>
+    <VersionPrefix>5.0.5</VersionPrefix>
 
     <!-- Packing -->
     <Copyright>Copyright 2020 © The Npgsql Development Team</Copyright>

Directory.Build.targets

@@ -9,7 +9,7 @@
     <PackageReference Update="Microsoft.CodeAnalysis.PublicApiAnalyzers" Version="3.3.1" />
 
     <!-- Plugins -->
-    <PackageReference Update="NetTopologySuite.IO.PostGIS" Version="2.0.0" />
+    <PackageReference Update="NetTopologySuite.IO.PostGIS" Version="2.1.0" />
     <PackageReference Update="NodaTime" Version="3.0.1" />
     <PackageReference Update="GeoJSON.Net" Version="1.1.73" />
     <PackageReference Update="Newtonsoft.Json" Version="12.0.2" />

Npgsql.sln.DotSettings

@@ -91,6 +91,7 @@
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=NOEXPORT/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=Npgsql/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=Npgsql_0027s/@EntryIndexedValue">True</s:Boolean>
+	<s:Boolean x:Key="/Default/UserDictionary/Words/=pgpass/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=PGTZ/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=Postgis/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=Postgre/@EntryIndexedValue">True</s:Boolean>

src/Npgsql/ConnectorPool.Multiplexing.cs

@@ -48,8 +48,8 @@ internal async Task BootstrapMultiplexing(NpgsqlConnection conn, NpgsqlTimeout t
             Debug.Assert(_multiplexing);
 
             var hasSemaphore = async
-                ? await _bootstrapSemaphore!.WaitAsync(timeout.TimeLeft, cancellationToken)
-                : _bootstrapSemaphore!.Wait(timeout.TimeLeft, cancellationToken);
+                ? await _bootstrapSemaphore!.WaitAsync(timeout.CheckAndGetTimeLeft(), cancellationToken)
+                : _bootstrapSemaphore!.Wait(timeout.CheckAndGetTimeLeft(), cancellationToken);
 
             // We've timed out - calling Check, to throw the correct exception
             if (!hasSemaphore)

src/Npgsql/ConnectorPool.cs

@@ -190,7 +190,7 @@ async ValueTask<NpgsqlConnector> RentAsync()
                 // served), which is crucial to us.
                 using var linkedSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
                 var finalToken = linkedSource.Token;
-                linkedSource.CancelAfter(timeout.TimeLeft);
+                linkedSource.CancelAfter(timeout.CheckAndGetTimeLeft());
 
                 while (true)
                 {
@@ -343,6 +343,7 @@ bool CheckIdleConnector([NotNullWhen(true)] NpgsqlConnector? connector)
 
                     // In case there's a waiting attempt on the channel, we write a null to the idle connector channel
                     // to wake it up, so it will try opening (and probably throw immediately)
+                    // Statement order is important since we have synchronous completions on the channel.
                     IdleConnectorWriter.TryWrite(null);
 
                     throw;
@@ -399,10 +400,6 @@ void CloseConnector(NpgsqlConnector connector)
                 Log.Warn("Exception while closing connector", e, connector.Id);
             }
 
-            // If a connector has been closed for any reason, we write a null to the idle connector channel to wake up
-            // a waiter, who will open a new physical connection
-            IdleConnectorWriter.TryWrite(null);
-
             var i = 0;
             for (; i < _max; i++)
                 if (Interlocked.CompareExchange(ref _connectors[i], null, connector) == connector)
@@ -414,6 +411,12 @@ void CloseConnector(NpgsqlConnector connector)
 
             var numConnectors = Interlocked.Decrement(ref _numConnectors);
             Debug.Assert(numConnectors >= 0);
+
+            // If a connector has been closed for any reason, we write a null to the idle connector channel to wake up
+            // a waiter, who will open a new physical connection
+            // Statement order is important since we have synchronous completions on the channel.
+            IdleConnectorWriter.TryWrite(null);
+
             // Only turn off the timer one time, when it was this Close that brought Open back to _min.
             if (numConnectors == _min)
                 DisablePruning();

src/Npgsql/NpgsqlBinaryImporter.cs

@@ -317,10 +317,11 @@ async Task Write<T>([AllowNull] T value, NpgsqlParameter param, bool async, Canc
                 }
                 else
                 {
-                    if (!(param is NpgsqlParameter<T> typedParam))
+                    if (param is not NpgsqlParameter<T> typedParam)
                     {
                         _params[_column] = typedParam = new NpgsqlParameter<T>();
                         typedParam.NpgsqlDbType = param.NpgsqlDbType;
+                        param = typedParam;
                     }
                     typedParam.TypedValue = value;
                 }

src/Npgsql/NpgsqlCommand.cs

@@ -294,20 +294,20 @@ public bool AllResultTypesAreUnknown
 
         #region State management
 
-        int _state;
+        volatile int _state;
 
         /// <summary>
         /// The current state of the command
         /// </summary>
         internal CommandState State
         {
-            private get { return (CommandState)_state; }
+            get => (CommandState)_state;
             set
             {
                 var newState = (int)value;
                 if (newState == _state)
                     return;
-                Interlocked.Exchange(ref _state, newState);
+                _state = newState;
             }
         }
 
@@ -1032,10 +1032,20 @@ public override Task<int> ExecuteNonQueryAsync(CancellationToken cancellationTok
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         async Task<int> ExecuteNonQuery(bool async, CancellationToken cancellationToken)
         {
-            using var reader = await ExecuteReader(CommandBehavior.Default, async, cancellationToken);
-            while (async ? await reader.NextResultAsync(cancellationToken) : reader.NextResult()) ;
+            var reader = await ExecuteReader(CommandBehavior.Default, async, cancellationToken);
+            try
+            {
+                while (async ? await reader.NextResultAsync(cancellationToken) : reader.NextResult()) ;
 
-            return reader.RecordsAffected;
+                return reader.RecordsAffected;
+            }
+            finally
+            {
+                if (async)
+                    await reader.DisposeAsync();
+                else
+                    reader.Dispose();
+            }
         }
 
         #endregion Execute Non Query
@@ -1069,8 +1079,19 @@ async Task<int> ExecuteNonQuery(bool async, CancellationToken cancellationToken)
             if (!Parameters.HasOutputParameters)
                 behavior |= CommandBehavior.SequentialAccess;
 
-            using var reader = await ExecuteReader(behavior, async, cancellationToken);
-            return reader.Read() && reader.FieldCount != 0 ? reader.GetValue(0) : null;
+            var reader = await ExecuteReader(behavior, async, cancellationToken);
+            try
+            {
+                var read = async ? await reader.ReadAsync(cancellationToken) : reader.Read();
+                return read && reader.FieldCount != 0 ? reader.GetValue(0) : null;
+            }
+            finally
+            {
+                if (async)
+                    await reader.DisposeAsync();
+                else
+                    reader.Dispose();
+            }
         }
 
         #endregion Execute Scalar

src/Npgsql/NpgsqlConnection.cs

@@ -196,7 +196,7 @@ void GetPoolAndSettings()
             {
                 // If the pool we created was the one that ended up being stored we need to increment the appropriate counter.
                 // Avoids a race condition where multiple threads will create a pool but only one will be stored.
-                NpgsqlEventSource.Log.PoolCreated();
+                NpgsqlEventSource.Log.PoolCreated(newPool);
             }
 
             _pool = PoolManager.GetOrAdd(_connectionString, _pool);
@@ -207,8 +207,8 @@ internal Task Open(bool async, CancellationToken cancellationToken)
             CheckClosed();
             Debug.Assert(Connector == null);
 
-            Log.Trace("Opening connection...");
             FullState = ConnectionState.Connecting;
+            Log.Trace("Opening connection...");
 
             if (Settings.Multiplexing)
             {
@@ -243,6 +243,8 @@ async Task OpenAsync(CancellationToken cancellationToken2)
                 {
                     var timeout = new NpgsqlTimeout(TimeSpan.FromSeconds(ConnectionTimeout));
 
+                    var enlistToTransaction = Settings.Enlist ? Transaction.Current : null;
+
                     if (_pool == null) // Un-pooled connection (or user forgot to set connection string)
                     {
                         if (string.IsNullOrEmpty(_connectionString))
@@ -258,24 +260,15 @@ async Task OpenAsync(CancellationToken cancellationToken2)
                     {
                         _userFacingConnectionString = _pool.UserFacingConnectionString;
 
-                        if (Settings.Enlist && Transaction.Current is Transaction transaction)
+                        // First, check to see if we there's an ambient transaction, and we have a connection enlisted
+                        // to this transaction which has been closed. If so, return that as an optimization rather than
+                        // opening a new one and triggering escalation to a distributed transaction.
+                        // Otherwise just get a new connector and enlist below.
+                        if (enlistToTransaction is not null && _pool.TryRentEnlistedPending(enlistToTransaction, out connector))
                         {
-                            // First, check to see if we there's an ambient transaction, and we have a connection enlisted
-                            // to this transaction which has been closed. If so, return that as an optimization rather than
-                            // opening a new one and triggering escalation to a distributed transaction.
-                            // Otherwise just get a new connector and enlist.
-                            if (_pool.TryRentEnlistedPending(transaction, out connector))
-                            {
-                                connector.Connection = this;
-                                EnlistedTransaction = transaction;
-                            }
-                            else
-                            {
-                                connector = await _pool.Rent(this, timeout, async, cancellationToken2);
-                                ConnectorBindingScope = ConnectorBindingScope.Connection;
-                                Connector = connector;
-                                EnlistTransaction(Transaction.Current);
-                            }
+                            connector.Connection = this;
+                            EnlistedTransaction = enlistToTransaction;
+                            enlistToTransaction = null;
                         }
                         else
                             connector = await _pool.Rent(this, timeout, async, cancellationToken2);
@@ -287,6 +280,9 @@ async Task OpenAsync(CancellationToken cancellationToken2)
                     ConnectorBindingScope = ConnectorBindingScope.Connection;
                     Connector = connector;
 
+                    if (enlistToTransaction is not null)
+                        EnlistTransaction(enlistToTransaction);
+
                     // Since this connector was last used, PostgreSQL types (e.g. enums) may have been added
                     // (and ReloadTypes() called), or global mappings may have changed by the user.
                     // Bring this up to date if needed.
@@ -334,7 +330,6 @@ void CompleteOpen()
             {
                 Log.Debug("Connection opened (multiplexing)");
                 FullState = ConnectionState.Open;
-                OnStateChange(ClosedToOpenEventArgs);
             }
         }
 
@@ -471,7 +466,20 @@ public ConnectionState FullState
                         },
                     _ => _fullState
                 };
-            internal set => _fullState = value;
+            internal set
+            {
+                var originalOpen = _fullState.HasFlag(ConnectionState.Open);
+
+                _fullState = value;
+
+                var currentOpen = _fullState.HasFlag(ConnectionState.Open);
+                if (currentOpen != originalOpen)
+                {
+                    OnStateChange(currentOpen
+                        ? ClosedToOpenEventArgs
+                        : OpenToClosedEventArgs);
+                }
+            }
         }
 
         /// <summary>
@@ -483,11 +491,13 @@ public override ConnectionState State
         {
             get
             {
-                var s = FullState;
-                if ((s & ConnectionState.Open) != 0)
-                    return ConnectionState.Open;
-                if ((s & ConnectionState.Connecting) != 0)
+                var fullState = FullState;
+                if (fullState.HasFlag(ConnectionState.Connecting))
                     return ConnectionState.Connecting;
+
+                if (fullState.HasFlag(ConnectionState.Open))
+                    return ConnectionState.Open;
+
                 return ConnectionState.Closed;
             }
         }
@@ -718,10 +728,11 @@ internal Task Close(bool async, CancellationToken cancellationToken = default)
                 // TODO: Consider falling through to the regular reset logic. This adds some unneeded conditions
                 // and assignment but actual perf impact should be negligible (measure).
                 Debug.Assert(Connector == null);
+                Volatile.Write(ref _closing, 0);
+
                 FullState = ConnectionState.Closed;
                 Log.Debug("Connection closed (multiplexing)");
-                OnStateChange(OpenToClosedEventArgs);
-                Volatile.Write(ref _closing, 0);
+
                 return Task.CompletedTask;
             }
 
@@ -776,7 +787,11 @@ async Task CloseAsync(CancellationToken cancellationToken)
                 else
                 {
                     if (_pool == null)
+                    {
+                        // We're already doing the same in the NpgsqlConnector.Reset for pooled connections
+                        connector.Transaction?.UnbindIfNecessary();
                         connector.Close();
+                    }
                     else
                     {
                         // Clear the buffer, roll back any pending transaction and prepend a reset message if needed
@@ -802,7 +817,6 @@ async Task CloseAsync(CancellationToken cancellationToken)
                 ConnectorBindingScope = ConnectorBindingScope.None;
                 FullState = ConnectionState.Closed;
                 Log.Debug("Connection closed", connector.Id);
-                OnStateChange(OpenToClosedEventArgs);
             }
         }
 

src/Npgsql/NpgsqlConnectionStringBuilder.cs

@@ -940,6 +940,7 @@ public int InternalCommandTimeout
 
         /// <summary>
         /// The time to wait (in milliseconds) while trying to read a response for a cancellation request for a timed out or cancelled query, before terminating the attempt and generating an error.
+        /// Zero for infinity, -1 to skip the wait.
         /// Defaults to 2000 milliseconds.
         /// </summary>
         [Category("Timeouts")]
@@ -952,8 +953,8 @@ public int CancellationTimeout
             get => _cancellationTimeout;
             set
             {
-                if (value < 0)
-                    throw new ArgumentOutOfRangeException(nameof(value), value, $"{nameof(CancellationTimeout)} can't be negative");
+                if (value < -1)
+                    throw new ArgumentOutOfRangeException(nameof(value), value, $"{nameof(CancellationTimeout)} can't less than -1");
 
                 _cancellationTimeout = value;
                 SetValue(nameof(CancellationTimeout), value);

src/Npgsql/NpgsqlConnector.Auth.cs

@@ -460,12 +460,7 @@ class AuthenticationCompleteException : Exception { }
             if (password != null)
                 return password;
 
-            var passFile = Settings.Passfile ?? PostgresEnvironment.PassFile;
-            if (passFile is null && PostgresEnvironment.PassFileDefault is string passFileDefault)
-            {
-                passFile = passFileDefault;
-            }
-
+            var passFile = Settings.Passfile ?? PostgresEnvironment.PassFile ?? PostgresEnvironment.PassFileDefault;
             if (passFile != null)
             {
                 var matchingEntry = new PgPassFile(passFile!)