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

SQL Client Upgrade from 5.1.1 to 5.1.2 or Later Causes Selenium Tests to Run Slower #2481

Closed
johncrodriguez96 opened this issue May 3, 2024 · 10 comments

Comments

@johncrodriguez96
Copy link

johncrodriguez96 commented May 3, 2024

Describe the bug

Our team has narrowed down the reason for our build timeouts to this newer version of SQL Client.
Below are images of tests with timestamps related to this package.

v5.1.1 - Before upgrading
image

v5.1.2 - After upgrading
image

After a second run of the same tests, the time looks so much better. Could there be something being cached?
image

To reproduce

This is how our test SQL Connections are constructed. We use Selenium for unit testing. Below shows the SqlConnectionStringBuilder:

		var sqlConnectionStringBuilder = new SqlConnectionStringBuilder
		{
			DataSource = ".",
			InitialCatalog = nameOfSccServer,
			Encrypt = true,
			MultipleActiveResultSets = false,
			PersistSecurityInfo = false,
			TrustServerCertificate = true,
			ConnectTimeout = 30
		};

Here are more timestamps taken using dotTrace. It's a tool that shows the call stack made during a test run along with time stamps.
Below shows the call DbConnectionPool.TryGetConnection is much slower in the newer package
image

Expected behavior

The time results from the second run is what would be expected.

Further technical details

Microsoft.Data.SqlClient version: (found on the nuget or Microsoft.Data.SqlClient.dll)
.NET target: net8.0
SQL Server version: SQL Server 15.0.2101.7
Operating system: Windows 11 Pro Version 22H2

@roji
Copy link
Member

roji commented May 3, 2024

Likely a dup of dotnet/efcore#33399 (caused by #1983). The mitigation will probably be #2433.

@JRahnama JRahnama added the 🆕 Triage Needed For new issues, not triaged yet. label May 6, 2024
@kf-gonzalez2
Copy link

Need to revert PR #1983 in the 5.1 and 5.2 branches

@kf-gonzalez2 kf-gonzalez2 removed the 🆕 Triage Needed For new issues, not triaged yet. label May 7, 2024
@JRahnama
Copy link
Contributor

JRahnama commented May 11, 2024

Likely a dup of dotnet/efcore#33399 (caused by #1983). The mitigation will probably be #2433.

@roji, I am a bit confused. Do we really need to revert #1983?

@roji
Copy link
Member

roji commented May 11, 2024

@JRahnama I don't think so, I'm not sure what @kf-gonzalez2's reasoning is...

@ErikEJ
Copy link
Contributor

ErikEJ commented May 12, 2024

Is it not a question of getting #2433 finished and approved?

@David-Engel
Copy link
Contributor

The behavior change of #1983 was not intended in the 5.x line, particularly in the 5.1 backport. I consider it a breaking change. So the plan is to revert it in 5.x and bring it into 6.x where it will also include the connection override options to disable connection retries in order to maintain fast fail functionality.

@JRahnama
Copy link
Contributor

JRahnama commented Jun 1, 2024

Hotfix version 5.2.1 is released. Can you test with that version please?

@ajcvickers
Copy link
Member

ajcvickers commented Jun 4, 2024

Verified that the EF regression is fixed in 5.2.1.

@ErikEJ
Copy link
Contributor

ErikEJ commented Jun 4, 2024

@ajcvickers assume you mean 5.2.1 ?

@johncrodriguez96
Copy link
Author

It seems to have been fixed now. Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

7 participants