-
Notifications
You must be signed in to change notification settings - Fork 281
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merge common code bases for TdsParserStateObject.cs (4) #2254
Merge common code bases for TdsParserStateObject.cs (4) #2254
Conversation
…ject.netcore.cs (similarly to TdsParser.ReliabilitySection.Assert). Add "missing" calls to these two methods.
…ly in TdsParserStateObject.netcore.cs.
…serStateObject.netfx.cs.
…cs. Note IntPtr.Zero is equivalent to default(IntPtr).
…ReleasePacket in TdsParserStateObject.netfx.cs.
…rStateObject.netfx.cs.
d4fa9b6
to
0f14302
Compare
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 ReportAttention:
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
if (readPacket != IntPtr.Zero) | ||
{ | ||
// Be sure to release packet, otherwise it will be leaked by native. | ||
SNINativeMethodWrapper.SNIPacketRelease(readPacket); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -13,6 +13,11 @@ | |||
|
|||
namespace Microsoft.Data.SqlClient | |||
{ | |||
#if NETFRAMEWORK | |||
using PacketHandle = IntPtr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
For context on reliability see comments on #2168 My notes: UnsafeCreateTimer - moved in the same file, no changes netfx only: netcore only: over to you @David-Engel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Much thanks for this!
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 |
Thanks for the positive feedback. I have some questions/notes:
|
|
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. |
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'd go with option i.
|
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. |
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 | ||
{ | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you intend to eliminate SNIPacketRelease
method call, I don't see any reason for retaining the "try-finally" block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No objections.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
Show resolved
Hide resolved
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() | ||
{ | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the way you addressed the code differences. 👏🏼
LGTM, waiting for comments to be addressed and resolved. |
…arserStateObject.cs Co-authored-by: DavoudEshtehari <61173489+DavoudEshtehari@users.noreply.github.com>
…arserStateObject.cs Co-authored-by: DavoudEshtehari <61173489+DavoudEshtehari@users.noreply.github.com>
…arserStateObject.cs Co-authored-by: DavoudEshtehari <61173489+DavoudEshtehari@users.noreply.github.com>
I have addressed all comments. Along with the empty try-finally block, I have removed the PrepareConstrainedRegions call above it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kudus @panoskj 🎉
Thanks guys, I will prepare the next one. |
Slightly smaller please, reviewing is hard. |
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.