Code Merge | TdsParserStateObject alignment (#2073)

* Code Merge: Bring TdsParserStateObject closer together between netcore and netfx

* Address feedback and remove excessive trace entry

Author: David-Engel

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs

@@ -98,8 +98,9 @@ internal bool TryInitialize(TdsParserStateObject stateObj, int columnsCount)
                     return false;
                 }
 
-                SqlClientEventSource.Log.TryAdvancedTraceEvent("TdsParserStateObject.NullBitmap.Initialize | INFO | ADV | State Object Id {0}, NBCROW bitmap received, column count = {1}", stateObj._objectID, columnsCount);
-                SqlClientEventSource.Log.TryAdvancedTraceBinEvent("TdsParserStateObject.NullBitmap.Initialize | INFO | ADV | State Object Id {0}, Null Bitmap length {1}, NBCROW bitmap data: {2}", stateObj._objectID, (ushort)_nullBitmap.Length, _nullBitmap);
+                SqlClientEventSource.Log.TryAdvancedTraceEvent("TdsParserStateObject.NullBitmap.Initialize | INFO | ADV | State Object Id {0}, NBCROW bitmap received, column count = {1}", stateObj.ObjectID, columnsCount);
+                SqlClientEventSource.Log.TryAdvancedTraceBinEvent("TdsParserStateObject.NullBitmap.Initialize | INFO | ADV | State Object Id {0}, NBCROW bitmap data. Null Bitmap {1}, Null bitmap length: {2}", stateObj.ObjectID, _nullBitmap, (ushort)_nullBitmap.Length);
+
                 return true;
             }
         }
@@ -484,7 +485,7 @@ internal bool TryReadChar(out char value)
 
             AssertValidState();
             value = (char)((buffer[1] << 8) + buffer[0]);
-            
+
             return true;
         }
 
@@ -543,7 +544,6 @@ internal bool TryReadInt32(out int value)
             AssertValidState();
             value = (buffer[3] << 24) + (buffer[2] << 16) + (buffer[1] << 8) + buffer[0];
             return true;
-
         }
 
         // This method is safe to call when doing async without snapshot
@@ -564,7 +564,7 @@ internal bool TryReadInt64(out long value)
                 // then use ReadByteArray since the logic is there to take care of that.
 
                 int bytesRead;
-                if (!TryReadByteArray(_bTmp.AsSpan(_bTmpRead), 8 - _bTmpRead, out bytesRead))
+                if (!TryReadByteArray(_bTmp.AsSpan(start: _bTmpRead), 8 - _bTmpRead, out bytesRead))
                 {
                     Debug.Assert(_bTmpRead + bytesRead <= 8, "Read more data than required");
                     _bTmpRead += bytesRead;
@@ -642,7 +642,7 @@ internal bool TryReadUInt32(out uint value)
                 // then use ReadByteArray since the logic is there to take care of that.
 
                 int bytesRead;
-                if (!TryReadByteArray(_bTmp.AsSpan(_bTmpRead), 4 - _bTmpRead, out bytesRead))
+                if (!TryReadByteArray(_bTmp.AsSpan(start: _bTmpRead), 4 - _bTmpRead, out bytesRead))
                 {
                     Debug.Assert(_bTmpRead + bytesRead <= 4, "Read more data than required");
                     _bTmpRead += bytesRead;
@@ -1828,13 +1828,13 @@ public void ProcessSniPacket(PacketHandle packet, uint error)
                     }
 
                     SniReadStatisticsAndTracing();
-
+                    SqlClientEventSource.Log.TryAdvancedTraceBinEvent("TdsParser.ReadNetworkPacketAsyncCallback | INFO | ADV | State Object Id {0}, Packet read. In Buffer {1}, In Bytes Read: {2}", ObjectID, _inBuff, (ushort)_inBytesRead);
 
                     AssertValidState();
                 }
                 else
                 {
-                    throw SQL.ParsingError();
+                    throw SQL.ParsingError(ParsingErrorState.ProcessSniPacketFailed);
                 }
             }
         }
@@ -1896,7 +1896,6 @@ public void ReadAsyncCallback(IntPtr key, PacketHandle packet, uint error)
             //    to the outstanding GCRoot until AppDomain.Unload.
             // We live with the above for the time being due to the constraints of the current
             // reliability infrastructure provided by the CLR.
-            SqlClientEventSource.Log.TryTraceEvent("TdsParserStateObject.ReadAsyncCallback | Info | State Object Id {0}, received error {1} on idle connection", _objectID, (int)error);
 
             TaskCompletionSource<object> source = _networkPacketTaskSource;
 #if DEBUG
@@ -1919,7 +1918,7 @@ public void ReadAsyncCallback(IntPtr key, PacketHandle packet, uint error)
                 Debug.Assert(CheckPacket(packet, source) && source != null, "AsyncResult null on callback");
 
                 if (_parser.MARSOn)
-                { 
+                {
                     // Only take reset lock on MARS and Async.
                     CheckSetResetConnectionState(error, CallbackType.Read);
                 }
@@ -1928,10 +1927,10 @@ public void ReadAsyncCallback(IntPtr key, PacketHandle packet, uint error)
 
                 // The timer thread may be unreliable under high contention scenarios. It cannot be
                 // assumed that the timeout has happened on the timer thread callback. Check the timeout
-                // synchrnously and then call OnTimeoutSync to force an atomic change of state. 
+                // synchrnously and then call OnTimeoutSync to force an atomic change of state.
                 if (TimeoutHasExpired)
                 {
-                    OnTimeoutSync(true);
+                    OnTimeoutSync(asyncClose: true);
                 }
 
                 // try to change to the stopped state but only do so if currently in the running state
@@ -2125,6 +2124,13 @@ public void WriteAsyncCallback(IntPtr key, PacketHandle packet, uint sniError)
         /////////////////////////////////////////
         // Network/Packet Writing & Processing //
         /////////////////////////////////////////
+
+        //
+        // Takes a secure string and offsets and saves them for a write latter when the information is written out to SNI Packet
+        //  This method is provided to better handle the life cycle of the clear text of the secure string
+        //  This method also ensures that the clear text is not held in the unpined managed buffer so that it avoids getting moved around by CLR garbage collector
+        //  TdsParserStaticMethods.EncryptPassword operation is also done in the unmanaged buffer for the clear text later
+        //
         internal void WriteSecureString(SecureString secureString)
         {
             Debug.Assert(_securePasswords[0] == null || _securePasswords[1] == null, "There are more than two secure passwords");
@@ -2363,11 +2369,11 @@ internal Task WritePacket(byte flushMode, bool canAccumulate = false)
                 // However, since we don't know the version prior to login Is2005OrNewer was always false prior to login
                 // So removing the Is2005OrNewer check causes issues since the login packet happens to meet the rest of the conditions below
                 // So we need to avoid this check prior to login completing
-                state == TdsParserState.OpenLoggedIn &&
-                !_bulkCopyOpperationInProgress && // ignore the condition checking for bulk copy
-                    _outBytesUsed == (_outputHeaderLen + BitConverter.ToInt32(_outBuff, _outputHeaderLen))
+                state == TdsParserState.OpenLoggedIn
+                    && !_bulkCopyOpperationInProgress // ignore the condition checking for bulk copy
+                    && _outBytesUsed == (_outputHeaderLen + BitConverter.ToInt32(_outBuff, _outputHeaderLen))
                     && _outputPacketCount == 0
-                || _outBytesUsed == _outputHeaderLen
+                    || _outBytesUsed == _outputHeaderLen
                     && _outputPacketCount == 0)
             {
                 return null;
@@ -2420,6 +2426,7 @@ internal Task WritePacket(byte flushMode, bool canAccumulate = false)
                 // If we have been canceled, then ensure that we write the ATTN packet as well
                 task = AsyncHelper.CreateContinuationTask(task, CancelWritePacket);
             }
+
             return task;
         }
 
@@ -2443,7 +2450,7 @@ private void CancelWritePacket()
             }
         }
 
-#pragma warning disable 0420 // a reference to a volatile field will not be treated as volatile
+#pragma warning disable 420 // a reference to a volatile field will not be treated as volatile
 
         private Task SNIWritePacket(PacketHandle packet, out uint sniError, bool canAccumulate, bool callerHasConnectionLock, bool asyncClose = false)
         {
@@ -2456,9 +2463,7 @@ private Task SNIWritePacket(PacketHandle packet, out uint sniError, bool canAccu
 
             Task task = null;
             _writeCompletionSource = null;
-
             PacketHandle packetPointer = EmptyReadPacket;
-
             bool sync = !_parser._asyncWrite;
             if (sync && _asyncWriteCount > 0)
             { // for example, SendAttention while there are writes pending
@@ -2493,7 +2498,7 @@ private Task SNIWritePacket(PacketHandle packet, out uint sniError, bool canAccu
 
             if (sniError == TdsEnums.SNI_SUCCESS_IO_PENDING)
             {
-                Debug.Assert(!sync, "Completion should be handled in SniManagedWwrapper");
+                Debug.Assert(!sync, "Completion should be handled in SniManagedWrapper");
                 Interlocked.Increment(ref _asyncWriteCount);
                 Debug.Assert(_asyncWriteCount >= 0);
                 if (!canAccumulate)
@@ -2639,7 +2644,7 @@ internal void SendAttention(bool mustTakeWriteLock = false, bool asyncClose = fa
                             }
                         }
 #if DEBUG
-                }
+                    }
 #endif
 
                     SetTimeoutSeconds(AttentionTimeoutSeconds); // Initialize new attention timeout of 5 seconds.

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlDataReader.cs

@@ -860,7 +860,7 @@ private bool TryCleanPartialRead()
             }
 
 #if DEBUG
-            if (_stateObj._pendingData)
+            if (_stateObj.HasPendingData)
             {
                 byte token;
                 if (!_stateObj.TryPeekByte(out token))
@@ -1067,7 +1067,7 @@ private bool TryCloseInternal(bool closeReader)
 #else
                 {
 #endif //DEBUG
-                    if ((!_isClosed) && (parser != null) && (stateObj != null) && (stateObj._pendingData))
+                    if ((!_isClosed) && (parser != null) && (stateObj != null) && (stateObj.HasPendingData))
                     {
 
                         // It is possible for this to be called during connection close on a
@@ -1333,7 +1333,7 @@ private bool TryConsumeMetaData()
         {
             // warning:  Don't check the MetaData property within this function
             // warning:  as it will be a reentrant call
-            while (_parser != null && _stateObj != null && _stateObj._pendingData && !_metaDataConsumed)
+            while (_parser != null && _stateObj != null && _stateObj.HasPendingData && !_metaDataConsumed)
             {
                 if (_parser.State == TdsParserState.Broken || _parser.State == TdsParserState.Closed)
                 {
@@ -3473,7 +3473,7 @@ private bool TryHasMoreResults(out bool moreResults)
 
                 Debug.Assert(null != _command, "unexpected null command from the data reader!");
 
-                while (_stateObj._pendingData)
+                while (_stateObj.HasPendingData)
                 {
                     byte token;
                     if (!_stateObj.TryPeekByte(out token))
@@ -3563,7 +3563,7 @@ private bool TryHasMoreRows(out bool moreRows)
                         moreRows = false;
                         return true;
                 }
-                if (_stateObj._pendingData)
+                if (_stateObj.HasPendingData)
                 {
                     // Consume error's, info's, done's on HasMoreRows, so user obtains error on Read.
                     // Previous bug where Read() would return false with error on the wire in the case
@@ -3620,7 +3620,7 @@ private bool TryHasMoreRows(out bool moreRows)
                             moreRows = false;
                             return false;
                         }
-                        if (_stateObj._pendingData)
+                        if (_stateObj.HasPendingData)
                         {
                             if (!_stateObj.TryPeekByte(out b))
                             {
@@ -3964,7 +3964,7 @@ private bool TryReadInternal(bool setTimeout, out bool more)
                                 if (moreRows)
                                 {
                                     // read the row from the backend (unless it's an altrow were the marker is already inside the altrow ...)
-                                    while (_stateObj._pendingData)
+                                    while (_stateObj.HasPendingData)
                                     {
                                         if (_altRowStatus != ALTROWSTATUS.AltRow)
                                         {
@@ -3996,7 +3996,7 @@ private bool TryReadInternal(bool setTimeout, out bool more)
                                     }
                                 }
 
-                                if (!_stateObj._pendingData)
+                                if (!_stateObj.HasPendingData)
                                 {
                                     if (!TryCloseInternal(false /*closeReader*/))
                                     {
@@ -4020,7 +4020,7 @@ private bool TryReadInternal(bool setTimeout, out bool more)
                                 {
                                     // if we are in SingleRow mode, and we've read the first row,
                                     // read the rest of the rows, if any
-                                    while (_stateObj._pendingData && !_sharedState._dataReady)
+                                    while (_stateObj.HasPendingData && !_sharedState._dataReady)
                                     {
                                         if (!_parser.TryRun(RunBehavior.ReturnImmediately, _command, this, null, _stateObj, out _sharedState._dataReady))
                                         {
@@ -4061,7 +4061,7 @@ private bool TryReadInternal(bool setTimeout, out bool more)
                         more = false;
 
 #if DEBUG
-                        if ((!_sharedState._dataReady) && (_stateObj._pendingData))
+                        if ((!_sharedState._dataReady) && (_stateObj.HasPendingData))
                         {
                             byte token;
                             if (!_stateObj.TryPeekByte(out token))

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs

@@ -915,11 +915,11 @@ override internal void ValidateConnectionForExecute(SqlCommand command)
                     // or if MARS is off, then a datareader exists
                     throw ADP.OpenReaderExists(parser.MARSOn); // MDAC 66411
                 }
-                else if (!parser.MARSOn && parser._physicalStateObj._pendingData)
+                else if (!parser.MARSOn && parser._physicalStateObj.HasPendingData)
                 {
                     parser.DrainData(parser._physicalStateObj);
                 }
-                Debug.Assert(!parser._physicalStateObj._pendingData, "Should not have a busy physicalStateObject at this point!");
+                Debug.Assert(!parser._physicalStateObj.HasPendingData, "Should not have a busy physicalStateObject at this point!");
 
                 parser.RollbackOrphanedAPITransactions();
             }
@@ -1078,7 +1078,7 @@ private void ResetConnection()
             // obtains a clone.
 
             Debug.Assert(!HasLocalTransactionFromAPI, "Upon ResetConnection SqlInternalConnectionTds has a currently ongoing local transaction.");
-            Debug.Assert(!_parser._physicalStateObj._pendingData, "Upon ResetConnection SqlInternalConnectionTds has pending data.");
+            Debug.Assert(!_parser._physicalStateObj.HasPendingData, "Upon ResetConnection SqlInternalConnectionTds has pending data.");
 
             if (_fResetConnection)
             {