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

Unable to connect to SqlServer instance with managed SNI and Timeout=0 on SqlClient 5.0 #1733

Closed
vonzshik opened this issue Aug 22, 2022 · 2 comments · Fixed by #1742
Closed
Labels
💥 Regression Regression introduced from earlier PRs 🐛 Bug! Something isn't right !

Comments

@vonzshik
Copy link

Describe the bug

While attempting to establish the connection to SqlServer instance while having Timeout=0 in the connection string the app becomes unresponsive. I'm able to reproduce this problem on linux and windows with AppContext.SetSwitch("Switch.Microsoft.Data.SqlClient.UseManagedNetworkingOnWindows", true). It works fine on SqlClient 4.1.0.

To reproduce

AppContext.SetSwitch("Switch.Microsoft.Data.SqlClient.UseManagedNetworkingOnWindows", true);

var sw = Stopwatch.StartNew();
using var connection = new SqlConnection("Server=.;Database=master;Integrated Security=false;User ID=sa;Password=SomePassword;Pooling=true;Max Pool Size=1024;TrustServerCertificate=true;Timeout=0");
await connection.OpenAsync();

using var cmd = connection.CreateCommand();
cmd.CommandText = "SELECT 1";
await cmd.ExecuteNonQueryAsync();
Console.WriteLine($"Connection established successfully in {sw.ElapsedMilliseconds}ms. Press any key to continue...");
Console.ReadKey();

Expected behavior

SqlClient should be able to establish a connection with Timeout=0, just like in previous verions

Further technical details

Microsoft.Data.SqlClient 5.0
.NET target: .NET 6.0
SQL Server version: SQL Server 2017
Operating system: CentOS 7, Windows 10

Additional context

The problem is because while waiting for the dns, the timeout passed there is the default TimeSpan, which is 0, and that's why it fails immediately (it should have been -1):

TimeSpan ts = default(TimeSpan);
// In case the Timeout is Infinite, we will receive the max value of Int64 as the tick count
// The infinite Timeout is a function of ConnectionString Timeout=0
bool isInfiniteTimeOut = long.MaxValue == timerExpire;
if (!isInfiniteTimeOut)
{
ts = DateTime.FromFileTime(timerExpire) - DateTime.Now;
ts = ts.Ticks < 0 ? TimeSpan.FromTicks(0) : ts;
}

This was introduced in #1578.

@vonzshik
Copy link
Author

Just tested, changing that part of code like this makes it work:

Task<IPAddress[]> serverAddrTask = Dns.GetHostAddressesAsync(serverName);
if (isInfiniteTimeout)
{
	serverAddrTask.Wait();
}
else
{
	bool complete = serverAddrTask.Wait(timeout);

	// DNS timed out - don't block
	if (!complete)
		return null;
}

@lcheunglci
Copy link
Contributor

Thanks @vonzshik for bringing this to our attention. We'll have to discuss with the team and see if this is the desired behaviour or a regression and get back to you soon.

David-Engel added a commit to David-Engel/SqlClient that referenced this issue Aug 25, 2022
@JRahnama JRahnama added the 🐛 Bug! Something isn't right ! label Aug 25, 2022
@JRahnama JRahnama added the 💥 Regression Regression introduced from earlier PRs label Aug 25, 2022
David-Engel added a commit to David-Engel/SqlClient that referenced this issue Aug 25, 2022
Also fix SSRP when DataSource is an IP address
David-Engel added a commit that referenced this issue Aug 30, 2022
* Fix for issue #1733 - hang on infinite timeout

Also fix SSRP when DataSource is an IP address
DavoudEshtehari pushed a commit to DavoudEshtehari/SqlClient that referenced this issue 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 issue 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
💥 Regression Regression introduced from earlier PRs 🐛 Bug! Something isn't right !
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants