Possible copy-paste mistake in SqlConnection introduced by AAD Authentication work

[stijnherreman]

The work done for #19366 may have introduced a copy-paste mistake in SqlConnection. The change is present in the already released 2.2 (see dotnet/corefx#31039) and the planned release 3.0 (see dotnet/corefx#30342).

The possible mistake was noticed by @steji113 in the 2.2 PR. It appears that the change at https://github.com/dotnet/corefx/pull/31039/files#diff-78c5752d4a8ee85fdc3593a8851c86e9L1363 was copied to https://github.com/dotnet/corefx/pull/31039/files#diff-78c5752d4a8ee85fdc3593a8851c86e9L1406 and https://github.com/dotnet/corefx/pull/31039/files#diff-78c5752d4a8ee85fdc3593a8851c86e9L1444

Was this change meant to be done, or is it a mistake?

[Wraith2]

From what I can see I think this means there will be a lot of connection pool misses. Is that what you're seeing? lots of new connection throughput?

[stijnherreman]

I haven't switched that part of our code to CoreFX yet, so I don't know the impact. I created this issue to ensure it's being tracked, seeing as there's a lack of response in the PR itself.

[danmoseley]

Tagging @afsanehr @keeratsingh since as you point out, this already got into 2.2 bits so we should move quickly if it's going to cause a problem.

[saurabh500]

Replied dotnet/corefx#31039 (comment) based on the code in NetFx at https://github.com/Microsoft/referencesource/blob/master/System.Data/System/Data/SqlClient/SqlConnection.cs

Impact of this bug based on my understanding of the area:

1. The wrong connection pool will be cleared on a Change password using SecureString. As a result there will be impact on the functionality of the connections being used from the pool being cleared. (In case the wrong connection pool exists)
2. The right connection pool will not be cleared after a password change. That would lead to unused connections in the pool which will be cleared out eventually (3-5 minutes) to bring the connection pool count, down to Min Pool Size in case the connections from the pool is unused.

We should consider this for servicing.

[danmoseley]

Maybe it's fortunate then that few people have consumed this change yet, due to our bug: dotnet/corefx#33697

[Wraith2]

These don't cause a problem because they're all in dead code. All three ChangePassword overloads have no callers other than themselves. I've just commented them out and compiled the library just fine. The three real call sites through Connection_Set all pass both credential and token references are expected. So it looks buggy but isn't.

[saurabh500]

@Wraith2 It is buggy. You dont see any callers because these are public APIs

[saurabh500]

Which also means that we don't have tests for these APIs
I synced up with @afsanehr and she is taking care of this issue and boosting up the tests for this area.
Edit: I found some tests for successful password change, but nothing that checks the clearing of connection pool

[Wraith2]

Oh, that's a weird api. a static method that changes a connection string parameter password and affects the pool at the same time.

[saurabh500]

@steji113 and @stijnherreman Thanks for pointing this out.
Also it is good that you opened a new issue. A lot of times comments on Closed PRs and Closed Issue go un-monitored.

[steji113]

@saurabh500 absolutely, I should have thought to just open an issue initially. I also thought it seemed like it indicated a testing gap, as well. Thanks for the evaluation!

[danmoseley]

@stijnherreman thanks again.

[AfsanehR-zz]

I am going to close this since the fix is now both in master and Release/2.2 branch. Thank you @stijnherreman for finding this issue 👍