Skip to content
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 (5) #2302

Closed
wants to merge 3 commits into from

Conversation

panoskj
Copy link
Contributor

@panoskj panoskj commented Jan 18, 2024

This is a draft PR as I intend to add a few more commits. But let me know if you think this should be merged by itself.

So, in this comment, I asked about ValidateSNIConnection (3rd question, 4 possible solutions are suggested). @David-Engel responded and it looks like we both agree on the implementation of this PR.

To sum up, this PR effectively changes the implementation of IsConnectionAlive in netfx to check if the handle is null before passing it to SNICheckConnection. If it is null, the connection will be considered alive. This behavior is in line with netcore implementation for native.

Moreover, it appears that:

  1. Native SNICheckConnection doesn't expect null values.
  2. The handle in question will only be null if the TdsParserStateObject has been disposed.
  3. We assume IsConnectionAlive is not called if TdsParserStateObject has been disposed (otherwise you would have noticed the bug already).

Therefore, an even better implementation would be to throw an exception (or at least use a debug assertion) to make sure the handle is never null when CheckConnection is called. Note that netcore already does this for the managed implementation (which reinforces the assumption (3)). The null check below the linked line is redundant by the way (I guess the null check predated throwing the exception).

Let me know what you think @Wraith2 @DavoudEshtehari @JRahnama @David-Engel

…an exception: 1) there is no returned value and 2) the "error" variable is not used - there is no need to initialize it with SNI_SUCCESS.
Copy link

codecov bot commented Jan 18, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Project coverage is 66.67%. Comparing base (900d051) to head (3354c9c).
Report is 189 commits behind head on main.

Files with missing lines Patch % Lines
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2302      +/-   ##
==========================================
- Coverage   72.49%   66.67%   -5.83%     
==========================================
  Files         310      304       -6     
  Lines       61868    61544     -324     
==========================================
- Hits        44854    41036    -3818     
- Misses      17014    20508    +3494     
Flag Coverage Δ
addons ∅ <ø> (∅)
netcore 70.84% <90.90%> (-5.86%) ⬇️
netfx 64.36% <92.85%> (-5.59%) ⬇️

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.

@benrr101
Copy link
Contributor

benrr101 commented Oct 4, 2024

@panoskj Thanks for looking into merging this file. Since it's been sitting for so long, at this point, I think we'll close this PR and make a second attempt at it as part of a new effort to merge the projects.

@benrr101 benrr101 closed this Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants