Skip to content

Dev | Improved connection timeout#2098

Merged
JRahnama merged 6 commits intodotnet:mainfrom
DavoudEshtehari:Adhoc/TestOpenAsync
Aug 22, 2023
Merged

Dev | Improved connection timeout#2098
JRahnama merged 6 commits intodotnet:mainfrom
DavoudEshtehari:Adhoc/TestOpenAsync

Conversation

@DavoudEshtehari
Copy link
Contributor

@DavoudEshtehari DavoudEshtehari commented Jul 21, 2023 •
edited
Loading

  • Improved the connection timeout accuracy, by involving the IP address resolution and socket connection retry process on Managed SNI.
  • Expanded using the timeout timer on managed code.
  • Improved connection timeout tests.

@DavoudEshtehari DavoudEshtehari added the Area\Tests Issues that are targeted to tests or test projects label Jul 21, 2023
@DavoudEshtehari DavoudEshtehari added this to the 5.2.0-preview4 milestone Jul 21, 2023
@codecov
Copy link

codecov bot commented Jul 22, 2023 •
edited
Loading

Codecov Report

Patch coverage: 66.66% and project coverage change: -0.07% ⚠️

Comparison is base (610a2c2) 70.98% compared to head (4b1e43e) 70.92%.
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2098      +/-   ##
==========================================
- Coverage   70.98%   70.92%   -0.07%     
==========================================
  Files         305      305              
  Lines       61818    61844      +26     
==========================================
- Hits        43884    43864      -20     
- Misses      17934    17980      +46
Flag Coverage Δ
addons 92.88% <ø> (ø)
netcore 73.39% <62.10%> (-0.13%) ⬇️
netfx 69.72% <91.30%> (+<0.01%) ⬆️

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

Files Changed Coverage Δ
...oft/Data/SqlClient/TdsParserStateObject.netcore.cs 80.47% <ø> (-0.11%) ⬇️
...re/src/Microsoft/Data/SqlClient/SNI/SNINpHandle.cs 7.81% <6.66%> (-0.39%) ⬇️
...e/src/Microsoft/Data/SqlClient/SNI/SNITcpHandle.cs 63.01% <55.26%> (-0.40%) ⬇️
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs 68.28% <66.66%> (-0.39%) ⬇️
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs 71.06% <66.66%> (+0.03%) ⬆️
...t/netcore/src/Microsoft/Data/SqlClient/SNI/SSRP.cs 50.92% <80.00%> (-0.60%) ⬇️
...nt/src/Microsoft/Data/ProviderBase/TimeoutTimer.cs 83.78% <88.88%> (+1.64%) ⬆️
...core/src/Microsoft/Data/SqlClient/SNI/SNICommon.cs 56.66% <100.00%> (+1.53%) ⬆️
...tcore/src/Microsoft/Data/SqlClient/SNI/SNIProxy.cs 49.46% <100.00%> (ø)
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs 73.42% <100.00%> (+0.09%) ⬆️
... and 5 more

... and 6 files with indirect coverage changes

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

@DavoudEshtehari DavoudEshtehari added Area\Managed SNI Issues that are targeted to the Managed SNI codebase. and removed Area\Tests Issues that are targeted to tests or test projects labels Jul 28, 2023
@DavoudEshtehari DavoudEshtehari changed the title Test | Improved the connection timeout tests Fix | Improved the connection timeout on Managed SNI Jul 28, 2023
@David-Engel
Copy link
Contributor

David-Engel commented Aug 1, 2023 •
edited
Loading

Similar to the suggestion I made on the localdb PR, the managed SNI code would benefit from being passed the TimeoutTimer object. The long timerExpire in SNITcpHandle now came from one from a parent caller. You can use the MillisecondsRemaining method on the TimeoutTimer object to easily create the appropriate TimeSpan that you want to pass in as a timeout to .NET methods. And the IsExpired property can be used in the logic when you want to decide to throw instead of retry.

@DavoudEshtehari DavoudEshtehari changed the title Fix | Improved the connection timeout on Managed SNI Fix | Improved the connection timeout Aug 2, 2023
@DavoudEshtehari DavoudEshtehari added 💡 Enhancement and removed Area\Managed SNI Issues that are targeted to the Managed SNI codebase. labels Aug 2, 2023
@DavoudEshtehari DavoudEshtehari changed the title Fix | Improved the connection timeout Dev | Improved connection timeout Aug 2, 2023
David-Engel
David-Engel reviewed Aug 2, 2023
View reviewed changes
@DavoudEshtehari @David-Engel
Apply suggestions from code review
71a6278 
Co-authored-by: David Engel <dengel1012@gmail.com>
@JRahnama JRahnama merged commit 591d929 into dotnet:main Aug 22, 2023
@David-Engel David-Engel mentioned this pull request Jun 11, 2024
Closed
@JRahnama JRahnama mentioned this pull request Aug 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@David-Engel David-Engel David-Engel approved these changes

@Kaur-Parminder Kaur-Parminder Awaiting requested review from Kaur-Parminder

@arellegue arellegue Awaiting requested review from arellegue

+1 more reviewer

@JRahnama JRahnama JRahnama approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

5.2.0-preview4

Development

Successfully merging this pull request may close these issues.

3 participants

@DavoudEshtehari @David-Engel @JRahnama