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

Add | Use Minimum Login Timeout as 1 sec in .NET Core and enable behavior by default #2012

Merged
merged 2 commits into from
May 3, 2023

Conversation

cheenamalhotra
Copy link
Member

@cheenamalhotra cheenamalhotra commented Apr 20, 2023

This PR brings over app context switch "UseOneSecFloorInTimeoutCalculationDuringLogin" to use 1 second instead of 0 seconds as minimum login timeout to prevent indefinite timeouts and potential application hangs.

Also enables the app context switch by default, in both NetFx and NetCore.

NOTE: Public documentation for this App Context switch needs to enable support in .NET Core and .NET Standard when this change gets released.

@cheenamalhotra cheenamalhotra added the 🆕 Public API Issues/PRs that introduce new APIs to the driver. label Apr 20, 2023
@codecov
Copy link

codecov bot commented Apr 20, 2023

Codecov Report

Patch coverage: 73.33% and project coverage change: -0.68 ⚠️

Comparison is base (4a91443) 70.60% compared to head (6d92d78) 69.93%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2012      +/-   ##
==========================================
- Coverage   70.60%   69.93%   -0.68%     
==========================================
  Files         306      305       -1     
  Lines       61800    61802       +2     
==========================================
- Hits        43636    43222     -414     
- Misses      18164    18580     +416     
Flag Coverage Δ
addons 92.88% <ø> (ø)
netcore 73.42% <69.23%> (+0.05%) ⬆️
netfx 68.15% <66.66%> (-1.08%) ⬇️

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

Impacted Files Coverage Δ
...icrosoft/Data/SqlClient/LocalAppContextSwitches.cs 61.11% <63.63%> (+4.86%) ⬆️
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs 73.30% <100.00%> (-0.44%) ⬇️
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs 68.06% <100.00%> (ø)

... and 9 files with indirect coverage changes

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@DavoudEshtehari DavoudEshtehari added this to the 5.2.0-preview2 milestone Apr 20, 2023
@JRahnama
Copy link
Contributor

LGTM, but why are we adding an app context switch to address a bug? Whoever is facing this edge case situation must have suffered from it. Is this just to avoid breaking changes?

@David-Engel
Copy link
Contributor

LGTM, but why are we adding an app context switch to address a bug? Whoever is facing this edge case situation must have suffered from it. Is this just to avoid breaking changes?

The switch was already there. This just brings it to netcore and turns it on, by default. Keeping it around will let customers turn it off, just in case there was a good reason someone added it to netfx in the past. When we were talking about it, we couldn't think of a good reason the change was put under a context switch.

It seemed like there were two possible side effects of the change under the switch.

  • It could slightly extended a connect timeout time. (But who would actually notice this?)
  • Connection timeout errors could occur when slow connections right on the edge of your connect timeout might have previously succeeded beyond the timeout. (Fix: Extend your timeout if you were expecting this behavior.)

@JRahnama JRahnama changed the title Use Minimum Login Timeout as 1 sec in .NET Core and enable behavior by default Add | Use Minimum Login Timeout as 1 sec in .NET Core and enable behavior by default May 3, 2023
@JRahnama JRahnama merged commit 997bf38 into dotnet:main May 3, 2023
kant2002 pushed a commit to kant2002/SqlClient that referenced this pull request Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 Public API Issues/PRs that introduce new APIs to the driver.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants