Managed SNI for Windows #16589

saurabh500 · 2017-03-02T00:16:30Z

The changes include refactoring in SqlClient to build Managed SNI on Windows along with Native SNI.
This is the first step towards addressing #16129
The changes involve refactoring TdsParserStateObject into sub classes which contain the SNI specific logic and encapsulate the usage of SNIPacket and SNIHandle

There were changes done to extract parts of TdsParser into platform specific files so that the right behavior can be invoked.

The current behavior on Windows is under the flag TdsParserNativeObjectFactory.useManagedSni.
I will be adding changes to incorporate an AppContext flag to control the SNI behavior.

I ran the Functional and Manual tests to validate these changes.

cc @corivera @YoungGah @geleems

saurabh500 · 2017-03-02T01:05:39Z

Test Innerloop Windows_NT Debug Build and Test

stephentoub · 2017-03-02T12:44:29Z

src/System.Data.SqlClient/src/System/Data/SqlClient/LocalDBAPI.Windows.cs

+                    bool lockTaken = false;
+                    try
+                    {
+                        Monitor.Enter(s_dllLock, ref lockTaken);


The code is using:

bool lockTaken = false; try { Monitor.Enter(s_dllLock, ref lockTaken); ... // protected code } finally { if (lockTaken) Monitor.Exit(s_dllLock); }

That's exactly what lock does, so it could just be:

lock (s_dllLock) { ... // protected code }

stephentoub · 2017-03-02T12:45:08Z

src/System.Data.SqlClient/src/System/Data/SqlClient/LocalDBAPI.Windows.cs

+                            if (s_userInstanceDLLHandle != IntPtr.Zero)
+                            {
+                            }
+                            else


Why not if (s_userInstanceDLLHandle == IntPtr.Zero) rather than having an empty if block?

stephentoub · 2017-03-02T12:45:59Z

src/System.Data.SqlClient/src/System/Data/SqlClient/LocalDBAPI.Windows.cs

+                            {
+                                SNINativeMethodWrapper.SNI_Error sniError;
+                                SNINativeMethodWrapper.SNIGetLastError(out sniError);
+                                throw CreateLocalDBException(errorMessage: SR.GetString("LocalDB_FailedGetDLLHandle"), sniError: (int)sniError.sniError);


It'd be great to move to an SR approach that the rest of the repo uses. Is there a plan to do so?

There is no such formal plan to do so. I will move to SR in the LocalDB files

stephentoub · 2017-03-02T12:47:36Z

src/System.Data.SqlClient/src/System/Data/SqlClient/LocalDBAPI.cs

@@ -48,45 +48,7 @@ internal static void ReleaseDLLHandles()
        private static IntPtr s_userInstanceDLLHandle = IntPtr.Zero;

        private static object s_dllLock = new object();


Nit: readonly... I personally like to see readonly whenever possible, but in particular on lock objects, as if they're accidentally changed the results can be quite bad and hard to track down.

stephentoub · 2017-03-02T12:48:28Z

src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNIProxy.cs

        /// <param name="packet">SNI packet</param>
-        public void PacketReset(SNIHandle handle, bool write, SNIPacket packet)
+        public void PacketReset(SNIPacket packet)
        {
            packet.Reset();
        }


Is this method still needed, or could the callers just do packet.Reset() instead of proxy.PacketReset(packet)?

Looking at the calls to SNIPacket, I think the original idea was to have all operations to SNI internals like SNIPacket and SNIHandle to go through the SNIProxy. I wanted to leave it that way.

@corivera do you have any preferences here?

Just calling packet.Reset() in place of this method call seems fine. There's no complex logic in this method that merits keeping it.

stephentoub · 2017-03-02T12:49:29Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.Windows.cs

+            uint error = TdsEnums.SNI_SUCCESS;
+
+            try { }
+            finally


Nit: there aren't thread aborts in coreclr. You could get rid of the try { } finally.

@stephentoub

there aren't thread aborts in coreclr.

Is this one of the things which were removed and can be added in future or Thread Aborts have been replaced with something else which removes the need for empty try{} finally{ //Dosomething} pattern?

There's no plan for thread aborts to come back.
cc: @jkotas

I want to leave these try {} finally{..} blocks as is.
I understand that the reason for which this coding pattern was followed didn't exist, but the pattern means that the code in finally must execute.

I don't see any significant advantage of removing this pattern.

we have a tracking issue to remove all try{} code. so it would be better to fix this from now.

@tarekgh Can you provide the issue number?

https://github.com/dotnet/corefx/issues/15411

Thank you. Updated the PR

stephentoub · 2017-03-02T12:50:56Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.Windows.cs

+                    _pMarsPhysicalConObj.ReleasePacket(temp);
+                }
+            }
+            Debug.Assert(IntPtr.Zero == temp, "unexpected syncReadPacket without corresponding SNIPacketRelease");


I'm confused by this assert. The code just above it has code that checks whether temp is not zero, so why do we expect it to be zero here?

I moved this code from TdsParser.cs
I don't know exactly what the intention of the original author was, but it seems like the Assert was in place to hint that temp should not be empty after the finally block has executed. If it was non-empty, then a packet release would have happened and everything would be alright. If it was not empty then a Debug Assert would catch the problem.

stephentoub · 2017-03-02T12:52:10Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.Windows.cs

+{
+    sealed internal partial class TdsParser
+    {
+        private static bool s_fSSPILoaded = false; // bool to indicate whether library has been loaded


Technically this should be volatile.

Good point.

stephentoub · 2017-03-02T12:53:37Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs


 using MSS = Microsoft.SqlServer.Server;

 namespace System.Data.SqlClient
 {
+
+    struct SNIErrorDetails


Nit: internal

stephentoub · 2017-03-02T12:54:01Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs

    // The TdsParser Object controls reading/writing to the netlib, parsing the tds,
    // and surfacing objects to the user.
-    sealed internal class TdsParser
+    sealed internal partial class TdsParser


Nit: internal sealed rather than sealed internal

stephentoub · 2017-03-02T12:54:24Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs

    // The TdsParser Object controls reading/writing to the netlib, parsing the tds,
    // and surfacing objects to the user.
-    sealed internal class TdsParser
+    sealed internal partial class TdsParser
    {
        static TdsParser()
        {
            // For CoreCLR, we need to register the ANSI Code Page encoding provider before attempting to get an Encoding from a CodePage
            Encoding.RegisterProvider(CodePagesEncodingProvider.Instance);


@tarekgh, is this still necessary in .NET Core 2.0?

I think registering the encoding provider should be the app decision and not the library decision. having the library registering the provider means any app using the library will pay the cost of the encoding even if the app not using the non-Unicode encodings.

@stephentoub encoding handling didn't change in Net core 2.0. the only change we did is we have made the console support non-Unicode encodings but this is limited to the console only.

In reply to: 103917083 [](ancestors = 103917083)

cc @weshaggard

In reply to: 103972132 [](ancestors = 103972132,103917083)

@saurabh500 just want to make sure you have seen the comments here regarding using the encoding provider.

@tarekgh SqlClient needs to provide the encoding related abstractions to the consumers of Sql client based on the exchange with SqlServer.
I don't think it makes sense for SqlClient to expect consumers to set the encoding provider.
Am I missing something here? Regardless if this needs to be addresses I will open another issue for this item.
Let me know if my understanding is incorrect.

I am fine to track this as separate issue but I am still seeing the clinet consuming the library can decide about the encoding. if you have a client using Unicode only DBs why you want them pay for the encoding cost? this is similar to the networking libraries which is up to the consumer of the library to decide about the encoding but not forcing every app consume the library to pay for the encoding even if they are not using it or need it.

Opened an issue #16842

stephentoub · 2017-03-02T12:54:52Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs

@@ -80,7 +85,7 @@ private static Task CompletedTask

        internal Encoding _defaultEncoding = null;                  // for sql character data

-        private static EncryptionOptions s_sniSupportedEncryptionOption = SNILoadHandle.SingletonInstance.Options;
+        private static EncryptionOptions s_sniSupportedEncryptionOption = TdsParserStateObjectFactory.Singleton.EncryptionOptions;// SNILoadHandle.SingletonInstance.Options;


What does the comment mean?

Will Remove

stephentoub · 2017-03-02T12:58:58Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserSessionPool.cs

-                    {
-                        sessionHandle.Dispose();
-                    }
+                    session.DisposeHandle();


Nit: Could just be:

_cache[i]?.DisposeHandle();

stephentoub · 2017-03-02T12:59:44Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserSessionPool.cs

@@ -89,11 +85,7 @@ internal void BestEffortCleanup()
                TdsParserStateObject session = _cache[i];


GitHub won't let me comment on it, but the comment above says "This is called from a ThreadAbort - ensure that it can be run from a CER catch"... thread aborts don't exist in coreclr.

stephentoub · 2017-03-02T13:00:30Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObject.cs

@@ -64,9 +52,12 @@ sealed internal class TdsParserStateObject
        internal int _outBytesUsed = TdsEnums.HEADER_LEN; // number of bytes used in internal write buffer -
                                                          // - initialize past header
                                                          // In buffer variables
-        private byte[] _inBuff;                                      // internal read buffer - initialize on login
+        protected byte[] _inBuff;                                      // internal read buffer - initialize on login


Nit: comment indentation looks off

stephentoub · 2017-03-02T13:00:44Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObject.cs

-#endif // MANAGED_SNI
-        private object _writePacketLockObject = new object();        // Used to synchronize access to _writePacketCache and _pendingWritePackets
+
+        protected object _writePacketLockObject = new object();        // Used to synchronize access to _writePacketCache and _pendingWritePackets


stephentoub · 2017-03-02T13:01:15Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObject.cs

@@ -195,7 +175,7 @@ sealed internal class TdsParserStateObject
        internal SqlErrorCollection _preAttentionWarnings;

        volatile private TaskCompletionSource<object> _writeCompletionSource = null;
-        volatile private int _asyncWriteCount = 0;
+        volatile protected int _asyncWriteCount = 0;


Nit: style-wise, the visibility should come first, e.g. protected volatile instead of volatile protected

stephentoub · 2017-03-02T13:04:05Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectFactory.cs

+
+        public static readonly TdsParserStateObjectFactory Singleton = new TdsParserStateObjectFactory();
+
+        public EncryptionOptions EncryptionOptions {


Nit: brace style

stephentoub · 2017-03-02T13:04:49Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectManaged.cs

+        private SNIHandle _sessionHandle = null;              // the SNI handle we're to work on
+        private SNIPacket _sniPacket = null;                // Will have to re-vamp this for MARS
+        internal SNIPacket _sniAsyncAttnPacket = null;                // Packet to use to send Attn
+        private Dictionary<SNIPacket, SNIPacket> _pendingWritePackets = new Dictionary<SNIPacket, SNIPacket>(); // Stores write packets that have been sent to SNI, but have not yet finished writing (i.e. we are waiting for SNI's callback)


stephentoub · 2017-03-02T13:04:57Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectManaged.cs

+        internal SNIPacket _sniAsyncAttnPacket = null;                // Packet to use to send Attn
+        private Dictionary<SNIPacket, SNIPacket> _pendingWritePackets = new Dictionary<SNIPacket, SNIPacket>(); // Stores write packets that have been sent to SNI, but have not yet finished writing (i.e. we are waiting for SNI's callback)
+
+        protected WritePacketCache _writePacketCache = new WritePacketCache(); // Store write packets that are ready to be re-used


stephentoub · 2017-03-02T13:05:40Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectManaged.cs

+                    if (asyncAttnPacket != null)
+                    {
+                        asyncAttnPacket.Dispose();
+                    }


Nit:

packetHandle?.Dispose(); asyncAttnPacket?.Dispose();

stephentoub · 2017-03-02T13:05:56Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectManaged.cs

+            if (null != sessionHandle || null != packetHandle)
+            {
+                try { }
+                finally


Nit: same try/finally thread abort comment as earlier

stephentoub · 2017-03-02T13:06:41Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectManaged.cs

+
+        internal override void DisposePacketCache()
+        {
+            if (_writePacketCache != null)


In what situation will this be null?

stephentoub · 2017-03-02T13:07:21Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectManaged.cs

+        internal override object ReadAsync(out uint error, ref object handle)
+        {
+            SNIPacket packet = null;
+            error = SNIProxy.Singleton.ReadAsync((SNIHandle)handle, ref packet);


Should ReadAsync take the packet via out rather than ref?

stephentoub · 2017-03-02T13:08:18Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectNative.cs

+                    if (asyncAttnPacket != null)
+                    {
+                        asyncAttnPacket.Dispose();
+                    }


Same nit as earlier

stephentoub · 2017-03-02T13:08:42Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectNative.cs

+
+        internal override void DisposePacketCache()
+        {
+            if (_writePacketCache != null)


Same question as earlier

stephentoub · 2017-03-02T13:09:01Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectNative.cs

+            if (sessionHandle != null)
+            {
+                sessionHandle.Dispose();
+            }


Nit:

Handle?.Dispose();

I think I can get rid of DisposeHandle() This is called by BestEffortCleanUp from the TdsParserSessionPool.cs which is not called by any caller.
The BestEffortCleanUp existed in NetFx for Thread Aborts.

corivera · 2017-03-03T00:14:14Z

src/System.Data.SqlClient/src/System/Data/SqlClient/LocalDBAPI.cs

-                    try
-                    {
-                        Monitor.Enter(s_dllLock, ref lockTaken);
+                    lock (s_dllLock) { 


You should do an auto-format pass on the changed files (for spacing, tabs, etc; this curly brace).

corivera · 2017-03-03T00:21:41Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectFactory.cs

+
+namespace System.Data.SqlClient
+{
+    internal sealed class TdsParserStateObjectFactory


Should the filename for this class be TdsParserStateObjectFactory.Windows.cs ?

corivera · 2017-03-08T00:36:32Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectFactory.cs

+        {
+            get
+            {
+                return useManagedSni ? SNILoadHandle.SingletonInstance.Status : SNI.SNILoadHandle.SingletonInstance.Status;


Should the SNI.SNILoadHandle come first here?

corivera · 2017-03-08T01:05:47Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectNative.cs

+    {
+        private SNIHandle _sessionHandle = null;              // the SNI handle we're to work on
+
+        private SNIPacket _sniPacket = null;                // Will have to re-vamp this for MARS


Do we still need this comment at the end of this line? Not sure what it's referring to.

The comment with this field declaration. Looks like a sort of TODO for MARS.
This has propagated since the time the SqlClient code was ported to CoreFx and has been in the framework code as well.

I don't think this is going to be changed or revamped.

corivera · 2017-03-08T01:18:53Z

src/System.Data.SqlClient/src/System/Data/SqlClient/LocalDBAPI.Windows.cs

+                if (s_userInstanceDLLHandle == IntPtr.Zero)
+                {
+
+                    lock (s_dllLock) { 


Curly brace on new line

corivera · 2017-03-08T01:22:47Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.Windows.cs

+
+namespace System.Data.SqlClient
+{
+    sealed internal partial class TdsParser


sealed comes after internal.

corivera · 2017-03-08T01:38:38Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectManaged.cs

+        {
+            lock (_writePacketLockObject)
+            {
+                try { }


Do we still need this empty try-finally?

corivera · 2017-03-08T01:39:29Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectNative.cs

+
+            if (null != sessionHandle || null != packetHandle)
+            {
+                try { }


Empty try-finally

corivera · 2017-03-08T01:39:55Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObjectNative.cs

+        {
+            lock (_writePacketLockObject)
+            {
+                try { }


Empty try-finally

corivera · 2017-03-08T01:41:57Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.cs


+                            WaitForSSLHandShakeToComplete(ref error);
+
                            // create a new packet encryption changes the internal packet size
                            try { } // EmptyTry/Finally to avoid FXCop violation


Empty try-finally

corivera · 2017-03-08T01:52:30Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParserStateObject.cs

@@ -2443,23 +2300,19 @@ internal void ReadSni(TaskCompletionSource<object> completion)
                    ChangeNetworkPacketTimeout(msecsRemaining, Timeout.Infinite);
                }

-                SNIHandle handle = null;
+                object handle = null;

                try { }


empty try-finally

corivera · 2017-03-08T02:07:29Z

src/System.Data.SqlClient/src/System/Data/SqlClient/TdsParser.Windows.cs

+            IntPtr temp = IntPtr.Zero;
+            uint error = TdsEnums.SNI_SUCCESS;
+
+


Extra line here

corivera

Approved pending my other comments

Refactor code for Read async Cleaning up

objects in the respective sub classes

Formatting changes Changes for Code Review comments

Additional CR comments

Make Native and Managed SNI coexist on Windows and make Native SNI the default implementation of the networking interface. Commit migrated from dotnet/corefx@bb05af4

dnfclas added the cla-already-signed label Mar 2, 2017

saurabh500 requested a review from corivera March 2, 2017 00:16

danmoseley added this to the 2.0.0 milestone Mar 2, 2017

karelz added the area-System.Data.SqlClient label Mar 2, 2017

karelz assigned saurabh500 Mar 2, 2017

stephentoub reviewed Mar 2, 2017

View reviewed changes

corivera reviewed Mar 3, 2017

View reviewed changes

corivera reviewed Mar 8, 2017

View reviewed changes

corivera approved these changes Mar 8, 2017

View reviewed changes

saurabh500 added 6 commits March 7, 2017 18:43

Clean build all OSes

a1f88f5

Refactor code for Read async Cleaning up

Make Native SNI as default and put all the usage of SNI dependent

4931662

objects in the respective sub classes

Remove MANAGED_SNI from csproj

04ab3ac

Formatting changes Changes for Code Review comments

Passing packet to SNIProxy using out

7d1cc40

Remove empty Try Finally #15411

85c4532

Additional CR comments

Code Review comments

8a9c7ac

saurabh500 force-pushed the buildmanagedsniOnWindows branch from ab3b131 to 8a9c7ac Compare March 8, 2017 05:29

saurabh500 merged commit bb05af4 into dotnet:master Mar 8, 2017

saurabh500 deleted the buildmanagedsniOnWindows branch March 8, 2017 07:19

saurabh500 mentioned this pull request Jan 31, 2020

Consider removing the Encoding.RegisterProvider from SqlClient dotnet/runtime#20515

Closed

edwardneal mentioned this pull request May 19, 2025

Merge | TdsParserStateObject PacketHandle usage dotnet/SqlClient#3353

Merged

		@@ -48,45 +48,7 @@ internal static void ReleaseDLLHandles()
		private static IntPtr s_userInstanceDLLHandle = IntPtr.Zero;

		private static object s_dllLock = new object();

		@@ -89,11 +85,7 @@ internal void BestEffortCleanup()
		TdsParserStateObject session = _cache[i];


		public static readonly TdsParserStateObjectFactory Singleton = new TdsParserStateObjectFactory();

		public EncryptionOptions EncryptionOptions {

		IntPtr temp = IntPtr.Zero;
		uint error = TdsEnums.SNI_SUCCESS;

Managed SNI for Windows #16589

Managed SNI for Windows #16589

Uh oh!

Conversation

saurabh500 commented Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saurabh500 commented Mar 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saurabh500 Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

saurabh500 commented Mar 2, 2017 •

edited

Loading

saurabh500 Mar 2, 2017 •

edited

Loading

stephentoub Mar 2, 2017 •

edited

Loading