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

Fix | Take out the ignoreSniOpenTimeout in open connection #2067

Conversation

arellegue
Copy link
Contributor

@arellegue arellegue commented Jun 19, 2023

This PR fixes #1126. The ignoreSniOpenTimeout was removed from all function calls in opening a connection as suggested.

arellegue and others added 9 commits June 8, 2023 10:43
…t string Encrypt option into SqlConnectionEncryptionOption type.
…o convert string Encrypt option into SqlConnectionEncryptionOption type."

This reverts commit 5c505a1.
…o convert string Encrypt option into SqlConnectionEncryptionOption type."

This reverts commit 5c505a1.
… 15 less than not work?"

This reverts commit cde0430.
Added Unit Test to prove that the ignore timeout has been removed.
@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.65 🎉

Comparison is base (3cb9730) 70.01% compared to head (e76d99f) 70.67%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2067      +/-   ##
==========================================
+ Coverage   70.01%   70.67%   +0.65%     
==========================================
  Files         306      306              
  Lines       61962    61975      +13     
==========================================
+ Hits        43383    43800     +417     
+ Misses      18579    18175     -404     
Flag Coverage Δ
addons 92.88% <ø> (ø)
netcore 73.44% <100.00%> (-0.06%) ⬇️
netfx 69.30% <100.00%> (+1.05%) ⬆️

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

Impacted Files Coverage Δ
...tcore/src/Microsoft/Data/SqlClient/SNI/SNIProxy.cs 49.46% <ø> (ø)
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs 73.71% <ø> (+0.02%) ⬆️
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs 68.61% <ø> (-0.06%) ⬇️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs 80.17% <ø> (+0.03%) ⬆️
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs 68.39% <ø> (+0.06%) ⬆️
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs 70.95% <ø> (-0.07%) ⬇️
...oft/Data/SqlClient/TdsParserSafeHandles.Windows.cs 93.06% <ø> (-0.14%) ⬇️
...soft/Data/SqlClient/TdsParserStateObjectManaged.cs 79.72% <100.00%> (ø)
...osoft/Data/SqlClient/TdsParserStateObjectNative.cs 86.84% <100.00%> (-0.53%) ⬇️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs 64.76% <100.00%> (+36.90%) ⬆️

... and 15 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@JRahnama JRahnama marked this pull request as draft June 20, 2023 17:18
@David-Engel
Copy link
Contributor

The original comment around this area of code is:

// UNDONE: (DB Mirroring Reconnect) Old behavior of not truly honoring timeout presevered 
// for non-failover, non-MSF scenarios to avoid breaking changes as part of a QFE.  Consider fixing timeout
// handling in next full release and removing ignoreSniOpenTimeout parameter.

It's not clear why anyone would want to ignore the specified timeout just from this comment. I would think users would want the ability to have a responsive timeout, so I'm inclined to be okay with "fixing" this issue.

@Kaur-Parminder Kaur-Parminder marked this pull request as ready for review June 22, 2023 18:48
@arellegue arellegue changed the title Issue 1126 Net Core Sql connection connect timeout less than 15 not working Fix | 1126 Net Core Sql connection connect timeout less than 15 is getting ignored Jun 26, 2023
@arellegue arellegue changed the title Fix | 1126 Net Core Sql connection connect timeout less than 15 is getting ignored Fix | Net Core Sql connection connect timeout less than 15 is getting ignored Jun 26, 2023
@DavoudEshtehari DavoudEshtehari added this to the 5.2.0-preview3 milestone Jun 26, 2023
@DavoudEshtehari DavoudEshtehari changed the title Fix | Net Core Sql connection connect timeout less than 15 is getting ignored Fix | Take the ignoreSniOpenTimeout out Net Core Sql connection connect timeout less than 15 is getting ignored Jul 11, 2023
@DavoudEshtehari DavoudEshtehari changed the title Fix | Take the ignoreSniOpenTimeout out Net Core Sql connection connect timeout less than 15 is getting ignored Fix | Take the ignoreSniOpenTimeout out in open connection Jul 11, 2023
@DavoudEshtehari DavoudEshtehari changed the title Fix | Take the ignoreSniOpenTimeout out in open connection Fix | Take out the ignoreSniOpenTimeout in open connection Jul 11, 2023
Copy link
Member

@DavoudEshtehari DavoudEshtehari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why you kept 'ignoreSniOpenTimeout' in netfx if you're not going to remove them?

@JRahnama JRahnama merged commit 7314307 into dotnet:main Jul 20, 2023
132 checks passed
DavoudEshtehari added a commit to DavoudEshtehari/SqlClient that referenced this pull request Jul 21, 2023
…otnet#2067)"

This reverts commit 7314307.

# Conflicts:
#	src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs
DavoudEshtehari added a commit to DavoudEshtehari/SqlClient that referenced this pull request Jul 25, 2023
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.

.NET Core SqlConnection ConnectTimout 15 less than not work?
4 participants