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] Hang on infinite timeout and managed SNI #1742

Merged
merged 2 commits into from
Aug 30, 2022

Conversation

David-Engel
Copy link
Contributor

@David-Engel David-Engel commented Aug 25, 2022

Fixes #1733 - regression introduced in #1578

I moved DNS calls into a common function and added logging for it. I decided not to use async DNS calls because the timeout logic is just wrong and I wanted to limit this fix to the issue at hand. The timeout logic will be addressed in a future PR.

I did notice and fix 2 issues in the SSRP logic right around the same DNS logic being fixed.

  1. The logic for when the hostName is an IP address was missing, resulting in a NRE.
  2. The cancellation token wasn't being associated with the parallelized UDP tasks so they weren't actually getting quickly canceled when Cancel() was called.

Also fix SSRP when DataSource is an IP address
@codecov
Copy link

codecov bot commented Aug 25, 2022

Codecov Report

Base: 71.50% // Head: 71.35% // Decreases project coverage by -0.14% ⚠️

Coverage data is based on head (88c697c) compared to base (bd21d76).
Patch coverage: 53.33% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1742      +/-   ##
==========================================
- Coverage   71.50%   71.35%   -0.15%     
==========================================
  Files         293      293              
  Lines       61385    61378       -7     
==========================================
- Hits        43892    43799      -93     
- Misses      17493    17579      +86     
Flag Coverage Δ
addons 92.38% <ø> (ø)
netcore 74.94% <53.33%> (-0.12%) ⬇️
netfx 69.22% <ø> (-0.11%) ⬇️

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

Impacted Files Coverage Δ
...t/netcore/src/Microsoft/Data/SqlClient/SNI/SSRP.cs 49.24% <33.33%> (-3.36%) ⬇️
...e/src/Microsoft/Data/SqlClient/SNI/SNITcpHandle.cs 59.25% <50.00%> (-3.96%) ⬇️
...core/src/Microsoft/Data/SqlClient/SNI/SNICommon.cs 69.35% <100.00%> (+2.11%) ⬆️
...ata/SqlClient/SqlConnectionTimeoutErrorInternal.cs 30.35% <0.00%> (-11.61%) ⬇️
...ActiveDirectoryAuthenticationTimeoutRetryHelper.cs 56.81% <0.00%> (-6.82%) ⬇️
...re/src/Microsoft/Data/SqlClient/SNI/SNINpHandle.cs 68.85% <0.00%> (-4.92%) ⬇️
...c/Microsoft/Data/SqlClient/SqlConnectionFactory.cs 68.85% <0.00%> (-0.82%) ⬇️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs 81.43% <0.00%> (-0.73%) ⬇️
.../Microsoft/Data/SqlClient/SNI/SNIMarsConnection.cs 76.72% <0.00%> (-0.63%) ⬇️
... and 14 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@DavoudEshtehari DavoudEshtehari added this to the 5.1.0-preview1 milestone Aug 29, 2022
@DavoudEshtehari DavoudEshtehari added the Ⓜ️ Managed SNI Use this label if the issue/PR relates to issues in Managed SNI label Aug 29, 2022
@David-Engel David-Engel merged commit ceaa33d into dotnet:main Aug 30, 2022
@David-Engel David-Engel deleted the 1733 branch August 30, 2022 16:32
DavoudEshtehari pushed a commit to DavoudEshtehari/SqlClient that referenced this pull request Aug 30, 2022
* Fix for issue dotnet#1733 - hang on infinite timeout

Also fix SSRP when DataSource is an IP address
DavoudEshtehari pushed a commit to DavoudEshtehari/SqlClient that referenced this pull request Aug 30, 2022
* Fix for issue dotnet#1733 - hang on infinite timeout

Also fix SSRP when DataSource is an IP address
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ⓜ️ Managed SNI Use this label if the issue/PR relates to issues in Managed SNI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to connect to SqlServer instance with managed SNI and Timeout=0 on SqlClient 5.0
3 participants