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

Fix | Fixes concurrent connection speed issues due to 'Authentication Context' cache not maintained in NetCore #466

Merged
merged 5 commits into from Mar 11, 2020

Conversation

David-Engel
Copy link
Contributor

@David-Engel David-Engel commented Mar 11, 2020

The authentication context cache of the connection pool was not being updated upon successful fedauth authentication in the netcore code. This was resulting in poor connection speed for subsequent connections which used AAD auth since they could not take advantage of the cached token. This code looks to have been missed when AAD authentication was ported from netfx to netcore.

This was found during investigation of #408

The new test should detect if subsequent connections are not using the cache. Hopefully connection times are consistent enough that this does not produce intermittent failures.

karinazhou and others added 5 commits February 13, 2020 16:16
- Added support for enclave simulator bypassing the actual attestation.
- Added BuildSimulator property to enable the simulator support.

How to use the enclave simulator:

Given Attestation Protocol = SIM and Enclave Attestation Url= SomeDummyURL in the connection string and run the AE enclave-enabled tests.

NOTE: This change is for internal testing only. The simulator support should be disabled in the official nuget package releases.
The authentication context cache of the connection pool was not being updated in the netcore code. This was resulting in poor connection speed for subsequent connections which used AAD auth since they could not take advantage of the cached token. This code looks to have been missed when AAD authentication was ported from netfx to netcore.
@Wraith2
Copy link
Contributor

Wraith2 commented Mar 11, 2020

It might be worth using private reflection to reach into the internals and find the cache to verify that it has been populated rather than relying on a timing artefact that could be flaky. I know private reflection is generally bad but as the owners you're allowed to assume your own internal details in this case I'd say.

@cheenamalhotra cheenamalhotra added this to the 2.0.0-preview2 milestone Mar 11, 2020
@cheenamalhotra cheenamalhotra added this to In progress in SqlClient v2.0.0 via automation Mar 11, 2020
@cheenamalhotra cheenamalhotra moved this from In progress to Review in progress in SqlClient v2.0.0 Mar 11, 2020
@cheenamalhotra cheenamalhotra changed the title Auth context cache Fix | Fixes concurrent connection speed issues due to 'Authentication Context' cache not maintained in NetCore Mar 11, 2020
@David-Engel
Copy link
Contributor Author

It might be worth using private reflection to reach into the internals and find the cache to verify that it has been populated rather than relying on a timing artefact that could be flaky. I know private reflection is generally bad but as the owners you're allowed to assume your own internal details in this case I'd say.

@Wraith2
I expect _dbConnectionPool.AuthenticationContexts.Count > 0 in SqlInternalConnectionTds after the first connection. I spent some time trying to wind my way through to view that via Reflection, but it was not apparent to me how to get to it. I'll have to revisit it later if I find the time. (If you have some time to spare, you're probably quicker than I am. 😁)

@David-Engel David-Engel merged commit 25447db into dotnet:master Mar 11, 2020
SqlClient v2.0.0 automation moved this from Review in progress to Done Mar 11, 2020
@David-Engel David-Engel deleted the authContextCache branch March 11, 2020 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Support parallel Physical Connection creation for multi-threaded scenarios
4 participants