Merge common code bases for TdsParserStateObject.cs (4) #2254

panoskj · 2023-12-04T11:17:04Z

More methods of TdsParserStateObject merged (#2168 follow-up).

The first two commits merge some identical methods. Then there is a series of commits refactoring netcore/netfx TdsParserStateObject in order to make more methods identical, without modifying their behavior. The last commit simply merges these methods.

…ject.netcore.cs (similarly to TdsParser.ReliabilitySection.Assert). Add "missing" calls to these two methods.

…ly in TdsParserStateObject.netcore.cs.

… always false).

…serStateObject.netfx.cs.

…cs. Note IntPtr.Zero is equivalent to default(IntPtr).

…ReleasePacket in TdsParserStateObject.netfx.cs.

…yncLocals).

…rStateObject.netfx.cs.

…StateObject.

panoskj · 2023-12-04T12:21:26Z

Force pushed to remove some whitespace changes I had accidentally included in the originally pushed commits - should make reviewing them a little bit easier when whitespaces comparison is on.

codecov · 2023-12-04T18:49:43Z

Codecov Report

Attention: 35 lines in your changes are missing coverage. Please review.

Comparison is base (416afcd) 72.45% compared to head (619bb4c) 72.44%.
Report is 24 commits behind head on main.

Files	Patch %	Lines
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs	85.98%	30 Missing ⚠️
...osoft/Data/SqlClient/TdsParserStateObject.netfx.cs	88.46%	3 Missing ⚠️
...oft/Data/SqlClient/TdsParserStateObject.netcore.cs	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2254      +/-   ##
==========================================
- Coverage   72.45%   72.44%   -0.02%     
==========================================
  Files         309      310       +1     
  Lines       61960    61868      -92     
==========================================
- Hits        44895    44818      -77     
+ Misses      17065    17050      -15

Flag	Coverage Δ
addons	`92.88% <ø> (ø)`
netcore	`76.57% <85.71%> (-0.14%)`	⬇️
netfx	`69.94% <86.63%> (+0.12%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

panoskj · 2023-12-12T11:49:24Z

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

-                        if (readPacket != IntPtr.Zero)
-                        {
-                            // Be sure to release packet, otherwise it will be leaked by native.
-                            SNINativeMethodWrapper.SNIPacketRelease(readPacket);


Since this code is unreachable, the enclosing try/finally serves no purpose other than whatever RuntimeHelpers.PrepareConstrainedRegions does. I have preserved the try/finally (by adding it in .net core) but I suspect it can be removed, because according to the documentation (emphasis mine):

The PrepareConstrainedRegions method must immediately precede a try block and marks catch, finally, and fault blocks as constrained execution regions.

If I understand correctly, RuntimeHelpers.PrepareConstrainedRegions only affects catch/finally and has no effect if they are empty.

panoskj · 2023-12-12T12:00:40Z

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

@@ -13,6 +13,11 @@

 namespace Microsoft.Data.SqlClient
 {
+#if NETFRAMEWORK
+    using PacketHandle = IntPtr;


An alternative would be to add a struct wrapping IntPtr instead of an alias, but would require refactoring a few more lines of code.

The advantage of using a struct is that the alias has to be defined in every file it is used, rather than once.

src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs

Wraith2 · 2024-01-10T18:39:56Z

For context on reliability see comments on #2168

My notes:

UnsafeCreateTimer - moved in the same file, no changes
SessionHandle - added in netfx with only a NativeHandle implementation because there is no managed equivalent in netfx
NullBitmap.TryInitialize - moved to shared, no changes
TrySkipLongBytes - moved to shared, no changes
TrySkipBytes - moved to shared, no changes
TryReadNetworkPacket - moved to shared, no changes. adds RuntimeHelpers.PrepareConstrainedRegion() stub implementation to netcore
PrepareReplaySnapshot - moved to shared, no changes
ReadSniSyncOverAsync - netcore version using handles is moved to shared, prepare and constrained region code added
OnConnectionClosed - moved to shared. the only change is in MemoryBarrier class and as previous discussed they are identical in functionality
SetTimeoutStateStopped - moved to shared, no changes
IsTimeoutStateExpired - moved to shared, no changes
OnTimeoutAsync - moved to shared, no changes
OnTimeoutSync - moved to shared, no changes
OnTimeoutCore - moved to shared. the only difference is a PrepareConstrainedRegion() call which is stubbed in netcore
ReadSni - netcore version using handled moved to shared and the try-finally and constrained region code added in. i think this is correct but please review it carefully.
IsConnectionAlive - merged code. carefully review. It looks like the readPacket variable is no longer used.

netfx only:
ReadSniError - changed to use packet handle instead of IntPtr
ProcessSniPacket - changed to use packet handle instead of IntPtr
ReadAsyncCallback - changed to use packet handle instead of IntPtr
WriteAsyncCallback - changed to use packet handle instead of IntPtr, changed to use interlocked memory barrier
SNIWritePacket - changed to use packet handle instead of IntPtr
IsValidPacket - added
EmptyReadPacket - added

netcore only:
ReadSniError - added reliability calls
WriteSecureString - added reliability calls
WriteByte - added reliability calls

over to you @David-Engel

David-Engel

LGTM. Much thanks for this!

David-Engel · 2024-01-12T01:42:39Z

Thanks @panoskj for the work. Thanks @Wraith2 for the notes. I went through it commit by commit and the merge looks nice and clean from that respect.

CC: @DavoudEshtehari , @JRahnama

panoskj · 2024-01-12T17:11:23Z

Thanks for the positive feedback. I have some questions/notes:

I would like to know what you think about removing this try/finally and CER (constrained execution region). It looks like there used to be another implementation that would read a packet which then had to be freed in the finally block and that's why the CER was needed. If it was solely up to me, I would remove it.
As a side note, I should point out the CERs place some constraints on the code being called (e.g. no boxing) in order to guarantee execution of the catch/finally blocks (even when the system is out of memory for example). It seems to me it is impossible to test the CERs honor these constraints and actually work as intended. The code being called inside CERs could have been modified to violate the CER constraints without anyone noticing. This is just a side note for future PRs - this PR is safe in this regard.
I was going to include the ValidateSNIConnection method in this PR. However, if you review it carefully, you will notice merging it is not that simple:
1. When called by IsConnectionAlive, the CheckConnection method that I added in netfx doesn't check for null SNIHandle before calling the native library (it simply calls it forwarding the null pointer).
2. But when called by ValidateSNIConnection, the netfx CheckConnection method should check if SNIHandle is null. If SNIHandle is null, CheckConnection should return SNI_SUCCESS resulting in ValidateSNIConnection returning true.
3. The netcore version checks if SNIHandle is null and returns SNI_SUCCESS if it is in both cases (when using the native wrapper that is).
4. It looks like SNIHandle will not be null in netfx, unless the TdsParserStateObject has been disposed.
Does anyone know what SNICheckConnection (native) does when given a null pointer? Is there any documentation about it? If it does something other than returning SNI_SUCCESS, then the netcore version behaves differently in this case (implying one of the two versions netcore/netfx is wrong). Unless this execution path is unreachable (e.g. if this method is never called on disposed TdsParserStateObjects due to other checks performed before calling it). Anyway, the options I can think of are:
1. Go with netcore logic (checking SNIHandle in both cases) in netfx too.
2. Use an #if directive to add the check prior to calling CheckConnection from ValidateSNIConnection for netfx.
3. Add a second CheckConnection method that is the same as the first in netcore but implements the slight difference in netfx.
4. Don't merge one of the two methods.
Which option would you choose?
Last but not least, where should I ask questions like the third one when I am preparing a new PR so that you guys (@Wraith2, @David-Engel and the rest of the team) can see them? I was think of either opening a draft PR or an issue mentioning you but none of these feels right - on the one hand I haven't made any commits to create a new PR yet (and the new PR would require this one as its base) and on the other hand this question isn't really an issue either.

Wraith2 · 2024-01-12T18:13:37Z

and 2) I think you should include but otherwise leave CER's alone unless it is causing a clear problem. While M.D.SqlClient is unlikely to ever be able to run in-proc in sql server someone somewhere may have a dependency on the exact behaviour of the CER containing code.
I'd need to dig into it in detail to even understand the question properly. I'd say open a specific small PR for that issue that changes it how you think is best and then we can discuss it there.
draft issue seems like the best idea, i've opened them on small changes in the past to get info and then closed if they weren't needed.

panoskj · 2024-01-12T19:45:21Z

The only problem about this CER is the finally clause is completely empty and (I think) it has no effect. Keeping it around can be confusing. Maybe we should consult someone who is more knowledgeable on the subject and either remove the unnecessary code or document why it is necessary, that's why I am asking.

As for the other question, it isn't really as complicated as it may seem when you read it, but you would have to read the method's code and maybe try to merge it in order to understand it. Looks like I should make a small draft PR so you can easily see what I am talking about.

David-Engel · 2024-01-12T23:16:23Z

1 and 2: I think we should try to keep the CERs. Wraith is correct in that MDS doesn't run inproc in SQL, but if SQL ever wants to (somehow?) move to current .NET in the SQL proc in the future, it's not going to be with SDS. So someone will have to do the big job of making sure MDS works in that context. It seems like removing the CERs could make that even harder. We can always decide to remove them later.

I'm not a C++ expert. But from what I do know and see in the native SNI code, the result will be unpredicatable. Thus I'm pretty sure you were correct here:

Unless this execution path is unreachable (e.g. if this method is never called on disposed TdsParserStateObjects due to other checks performed before calling it).

I'd go with option i.

Ditto on the draft PR. It will tell us exactly what you are considering in the code. If it's purely theoretical and pre-code, an issue is fine.

panoskj · 2024-01-13T09:18:59Z

Thanks for the feedback. In general I also agree we should keep CERs around, I was only referring to this specific case where it would not used even if running in-proc - that is dead code - but similar cases could be found later.

@David-Engel regarding the native SNI code, is it open-source? It would be a useful reference.

But if "this execution path is unreachable (e.g. if this method is never called on disposed TdsParserStateObjects due to other checks performed before calling it)" then the null check we are talking about is actually redundant, since the variable will never be null in this context. This would be better documented if an exception was thrown instead of returning SNI_SUCCESS from CheckConnection or at least if a debug assertion was in place.

This is what the managed CheckConnection does, but in a funny way: it calls GetSessionSNIHandleHandleOrThrow which returns a non-nullable reference or throws, then casts the returned reference to nullable and checks it against null (in which case it would return SNI_SUCCESS).

I guess my point is the code could be improved to cause less confusion and document any assumptions better. This isn't in the scope of these PRs, but in some cases like this, the netcore version has diverged from netfx and I have to understand what's going on anyway, so I could as well refactor the code to make it more self-documenting given the opportunity.

DavoudEshtehari · 2024-01-15T22:08:10Z

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

+                    try
+                    {
+                        TdsParser.ReliabilitySection.Assert("unreliable call to IsConnectionAlive");  // you need to setup for a thread abort somewhere before you call this method
+
+                        SniContext = SniContext.Snix_Connect;
+
+                        error = CheckConnection();
+                        if ((error != TdsEnums.SNI_SUCCESS) && (error != TdsEnums.SNI_WAIT_TIMEOUT))
+                        {
+                            // Connection is dead
+                            SqlClientEventSource.Log.TryTraceEvent("TdsParserStateObject.IsConnectionAlive | Info | State Object Id {0}, received error {1} on idle connection", _objectID, (int)error);
+                            isAlive = false;
+                            if (throwOnException)
+                            {
+                                // Get the error from SNI so that we can throw the correct exception
+                                AddError(_parser.ProcessSNIError(this));
+                                ThrowExceptionAndWarning();
+                            }
+                        }
+                        else
+                        {
+                            _lastSuccessfulIOTimer._value = DateTime.UtcNow.Ticks;
+                        }
+                    }
+                    finally
+                    {
+                    }


If you intend to eliminate SNIPacketRelease method call, I don't see any reason for retaining the "try-finally" block.

I agree, but note this change will require that the call to RuntimeHelpers.PrepareConstrainedRegions prior to it is removed too. This is what I was asking @Wraith2 and @David-Engel earlier but they didn't seem fond of the idea. Let me know how to proceed on this one.

EDIT: I figured I will remove the try-finally and the call to RuntimeHelpers.PrepareConstrainedRegions and if there is any objection I can revert the change. Otherwise the PR will be ready to be merged.

No objections.

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

DavoudEshtehari · 2024-01-15T22:18:24Z

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

+        private struct RuntimeHelpers
+        {
+            /// <summary>
+            /// This is a no-op in netcore version. Only needed for merging with netfx codebase.
+            /// </summary>
+            [Conditional("NETFRAMEWORK")]
+            internal static void PrepareConstrainedRegions() 
+            { 
+            }
+        }


I like the way you addressed the code differences. 👏🏼

JRahnama · 2024-01-16T23:24:39Z

LGTM, waiting for comments to be addressed and resolved.

…arserStateObject.cs Co-authored-by: DavoudEshtehari <61173489+DavoudEshtehari@users.noreply.github.com>

panoskj · 2024-01-17T08:38:08Z

I have addressed all comments. Along with the empty try-finally block, I have removed the PrepareConstrainedRegions call above it.

DavoudEshtehari

Kudus @panoskj 🎉

panoskj · 2024-01-17T19:58:00Z

Thanks guys, I will prepare the next one.

Wraith2 · 2024-01-17T20:06:22Z

Slightly smaller please, reviewing is hard.

panoskj added 14 commits December 1, 2023 17:55

Merge NullBitmap struct of TdsParserStateObject.

40d1351

Merge "Value Skip Logic" methods of TdsParserStateObject.

cae8c3f

Add RuntimeHelpers.PrepareConstrainedRegions stub in TdsParserStateOb…

fde2178

…ject.netcore.cs (similarly to TdsParser.ReliabilitySection.Assert). Add "missing" calls to these two methods.

Refactoring of ReadSni in TdsParserStateObject.netcore.cs.

53e4333

Add "missing" RuntimeHelpers.PrepareConstrainedRegions with try/final…

9076c6a

…ly in TdsParserStateObject.netcore.cs.

Remove unreachable code from TdsParserStateObject.netfx.cs (condition…

cedc2b5

… always false).

Replace Thread.MemoryBarrier with Interlocked.MemoryBarrier in TdsPar…

40a355c

…serStateObject.netfx.cs.

Add PacketHandle as an alias of IntPtr in TdsParserStateObject.netfx.…

2a9558d

…cs. Note IntPtr.Zero is equivalent to default(IntPtr).

Add TdsParserStateObjectFactory.UseManagedSNI, ReadSyncOverAsync and …

6a8f0d9

…ReleasePacket in TdsParserStateObject.netfx.cs.

Add SessionHandle and ReadAsync in TdsParserStateObject.netfx.cs.

e22c4e9

Port ADP.OnTimeoutAsync to netfx (fixes an edge case bug regarding As…

4a9ba4e

…yncLocals).

Add CheckConnection in TdsParserStateObject.netfx.cs.

a9b5585

Whitespace changes. Add a missing private access modifier in TdsParse…

e6cb4ab

…rStateObject.netfx.cs.

Merge most "Network/Packet Reading & Processing" methods of TdsParser…

0f14302

…StateObject.

panoskj force-pushed the MergeTdsParserStateObject-4 branch from d4fa9b6 to 0f14302 Compare December 4, 2023 12:16

panoskj changed the title ~~Merge common code bases for TdsParserStateObject.cs (3)~~ Merge common code bases for TdsParserStateObject.cs (4) Dec 4, 2023

JRahnama added this to the 5.2.0-preview5 milestone Dec 9, 2023

panoskj mentioned this pull request Dec 11, 2023

Reading large data (binary, text) asynchronously is extremely slow #593

Open

panoskj commented Dec 12, 2023

View reviewed changes

Wraith2 reviewed Jan 10, 2024

View reviewed changes

src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs Show resolved Hide resolved

NIT: Fix code formatting in UnsafeCreateTimer method.

e695cc7

David-Engel approved these changes Jan 12, 2024

View reviewed changes

David-Engel requested review from JRahnama, DavoudEshtehari, arellegue and H-Yeo January 12, 2024 01:42

DavoudEshtehari added the ➕ Code Health Changes related to source code improvements label Jan 15, 2024

DavoudEshtehari added this to In progress in SqlClient v5.2 via automation Jan 15, 2024

DavoudEshtehari reviewed Jan 15, 2024

View reviewed changes

JRahnama approved these changes Jan 16, 2024

View reviewed changes

SqlClient v5.2 automation moved this from In progress to Reviewer approved Jan 16, 2024

JRahnama self-requested a review January 16, 2024 23:22

panoskj and others added 4 commits January 17, 2024 10:01

Update src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsP…

7ca012a

…arserStateObject.cs Co-authored-by: DavoudEshtehari <61173489+DavoudEshtehari@users.noreply.github.com>

Update src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsP…

832e378

…arserStateObject.cs Co-authored-by: DavoudEshtehari <61173489+DavoudEshtehari@users.noreply.github.com>

Update src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsP…

240e242

…arserStateObject.cs Co-authored-by: DavoudEshtehari <61173489+DavoudEshtehari@users.noreply.github.com>

Removed dead code.

619bb4c

David-Engel approved these changes Jan 17, 2024

View reviewed changes

DavoudEshtehari approved these changes Jan 17, 2024

View reviewed changes

DavoudEshtehari merged commit 900d051 into dotnet:main Jan 17, 2024
148 checks passed

SqlClient v5.2 automation moved this from Reviewer approved to Done Jan 17, 2024

panoskj mentioned this pull request Jan 18, 2024

Merge common code bases for TdsParserStateObject.cs (5) #2302

Draft

twsouthwick pushed a commit to twsouthwick/SqlClient that referenced this pull request Jan 19, 2024

Merge common code bases for TdsParserStateObject.cs (4) (dotnet#2254)

326ab43

This was referenced Jan 25, 2024

Preview Release Notes 5.2.0-Preview5 #2316

Merged

Possible mergeable classes for merging code bases #1261

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge common code bases for TdsParserStateObject.cs (4) #2254

Merge common code bases for TdsParserStateObject.cs (4) #2254

panoskj commented Dec 4, 2023 •

edited

panoskj commented Dec 4, 2023

codecov bot commented Dec 4, 2023 •

edited

panoskj Dec 12, 2023 •

edited

panoskj Dec 12, 2023

Wraith2 commented Jan 10, 2024

David-Engel left a comment

David-Engel commented Jan 12, 2024

panoskj commented Jan 12, 2024 •

edited

Wraith2 commented Jan 12, 2024

panoskj commented Jan 12, 2024 •

edited

David-Engel commented Jan 12, 2024

panoskj commented Jan 13, 2024 •

edited

DavoudEshtehari Jan 15, 2024

panoskj Jan 17, 2024 •

edited

David-Engel Jan 17, 2024

DavoudEshtehari Jan 15, 2024

JRahnama commented Jan 16, 2024

panoskj commented Jan 17, 2024

DavoudEshtehari left a comment

panoskj commented Jan 17, 2024

Wraith2 commented Jan 17, 2024

Merge common code bases for TdsParserStateObject.cs (4) #2254

Merge common code bases for TdsParserStateObject.cs (4) #2254

Conversation

panoskj commented Dec 4, 2023 • edited

panoskj commented Dec 4, 2023

codecov bot commented Dec 4, 2023 • edited

Codecov Report

panoskj Dec 12, 2023 • edited

Choose a reason for hiding this comment

panoskj Dec 12, 2023

Choose a reason for hiding this comment

Wraith2 commented Jan 10, 2024

David-Engel left a comment

Choose a reason for hiding this comment

David-Engel commented Jan 12, 2024

panoskj commented Jan 12, 2024 • edited

Wraith2 commented Jan 12, 2024

panoskj commented Jan 12, 2024 • edited

David-Engel commented Jan 12, 2024

panoskj commented Jan 13, 2024 • edited

DavoudEshtehari Jan 15, 2024

Choose a reason for hiding this comment

panoskj Jan 17, 2024 • edited

Choose a reason for hiding this comment

David-Engel Jan 17, 2024

Choose a reason for hiding this comment

DavoudEshtehari Jan 15, 2024

Choose a reason for hiding this comment

JRahnama commented Jan 16, 2024

panoskj commented Jan 17, 2024

DavoudEshtehari left a comment

Choose a reason for hiding this comment

panoskj commented Jan 17, 2024

Wraith2 commented Jan 17, 2024

panoskj commented Dec 4, 2023 •

edited

codecov bot commented Dec 4, 2023 •

edited

panoskj Dec 12, 2023 •

edited

panoskj commented Jan 12, 2024 •

edited

panoskj commented Jan 12, 2024 •

edited

panoskj commented Jan 13, 2024 •

edited

panoskj Jan 17, 2024 •

edited