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

Old connection pool not cleaned up after token refresh? #2116

Closed
mamort opened this issue Aug 8, 2023 · 13 comments
Closed

Old connection pool not cleaned up after token refresh? #2116

mamort opened this issue Aug 8, 2023 · 13 comments
Labels
By Design By design

Comments

@mamort
Copy link

mamort commented Aug 8, 2023

We are using Azure AD auth and setting the token on the SQL connection using SQLConnection.AccessToken. It is my understanding that the token is part of the key for the connection pool so that when the token is refreshed a new connection pool is created. On our system it seems like the old connection pool remains with all its TCP connections open and seems to grow every time the token is refreshed. This from looking at the active-hard-connections performance counter. The performance counter number-of-active-connection-pools also keeps growing.

As a workaround I have tried calling SQLConnection.ClearPool(...) on a connection that belongs to the old pool a few minutes after the token is refreshed and this seems to work to close down the old pool properly.

Further technical details

Microsoft.Data.SqlClient version: 5.1.0
.NET target: .NET 7
SQL Server version: Azure SQL
Operating system: ubuntu-22.04

@JRahnama
Copy link
Member

JRahnama commented Aug 8, 2023

@mamort the token is cached inside the connection. Renewing the token does not clear the connection pool and does not create new connections. However, clearing connection pool will force acquiring new tokens as well. This is by design.

Update: Seems you are acquiring access token manually. In that case ignore my previous comment. Access token is a part of pool key and old pool will remain for 2-4 minutes or as long as other processes are still using that connection and/or access token.

@JRahnama JRahnama added this to Needs triage in SqlClient Triage Board via automation Aug 8, 2023
@JRahnama JRahnama added ⏳ Waiting for Customer We're waiting for your response :) and removed untriaged labels Aug 8, 2023
@JRahnama JRahnama moved this from Needs triage to Needs Investigation in SqlClient Triage Board Aug 8, 2023
@JRahnama JRahnama added By Design By design and removed ⏳ Waiting for Customer We're waiting for your response :) labels Aug 8, 2023
@mamort
Copy link
Author

mamort commented Aug 8, 2023

Thanks for the quick response. I wondered if this was taken care of in a better way when using the "Active Directory Default" auth mode. Might be a good idea to switch, but I wasn't sure if it supported WorkloadIdentity (?)

However, the way we do it now should work and the old pool does not get cleaned up after 2-4 minutes. I see the connections remain for days and do not seem to ever get closed. We create new connections for every query and dispose them right after using Dapper. Could be a bug in our code and I will check again, but it seemed unlikely (not many lines of code).

I guess the only way to know is to create a minimal example. Just too bad there is no way to force a token to be refreshed.

@JRahnama
Copy link
Member

JRahnama commented Aug 8, 2023

@mamort I am not familiar with WorkloadIdentity. Is it Azure Kubernetes Service related application?

@mamort
Copy link
Author

mamort commented Aug 9, 2023

Yes it is. It is described here:
https://learn.microsoft.com/en-us/azure/aks/workload-identity-overview

Since DefaultAzureCredential is used in SqlClient I think it should just work, but we need to be able to configure clientId among other things which might be possible by using environment variables

@mamort
Copy link
Author

mamort commented Aug 9, 2023

After looking at the SqlClient code it seems like the connection pool should be cleaned up in the DbConnectionPool in the PruneConnectionPoolGroups-method which in turn calls Prune() on the ConnectionPoolGroup. Pruning the ConnectionPoolGroup will only happen if the number of connections in the pool is zero. In our case it seems that the number of connections stay above zero for some reason although I am pretty sure we close/dispose the connections correctly.

A bit later I read this about connection pooling:
https://learn.microsoft.com/en-us/dotnet/framework/data/adonet/sql-server-connection-pooling

If Min Pool Size is either not specified in the connection string or is specified as zero, the connections in the pool will be closed after a period of inactivity. However, if the specified Min Pool Size is greater than zero, the connection pool is not destroyed until the AppDomain is unloaded and the process ends. Maintenance of inactive or empty pools involves minimal system overhead.

We have set MinPoolSize to 10 because we can get spikes in traffic where we immediately need more connections available and also when token refreshes and we get a new pool we want connections opened immediately. If MinPoolSize is 0 or 1 the "physical" connections get opened sequentially (ref: #408) which is not ideal.

So, to summarize, my current theory is this:

  1. We configure MinPoolSize to 10
  2. On first connection a pool is created for token T1 => 10 physical connections are opened
  3. Some time later, a new connection is requested, but now token is expired. Token gets refreshed => new pool is created with another set of 10 physical connections
  4. Pruning of the connection pool group happens, but since MinPoolSize is set to 10 it does not get cleaned up

And then this continues and the old connection pools that belong to expired tokens never gets closed down.

@mamort
Copy link
Author

mamort commented Aug 15, 2023

@JRahnama Any comments about this? Is this behavior just by design and something that must be handled by ourselves if we use the direct approach by setting the access token on the SqlConnection? Seems like bad design to not prune the old app pool using an expired token (when MinPoolSize > 0), but I guess it might be difficult to know that it has expired.

What happens when using "Active Directory Default" auth? Do you actively close down the old connection pool if the token has expired?

@JRahnama
Copy link
Member

JRahnama commented Aug 15, 2023

I guess it might be difficult to know that it has expired.

each token is good for almost 10 hours.

What happens when using "Active Directory Default" auth? Do you actively close down the old connection pool if the token has expired?

A new access token is acquired and assigned to the old connection inside the pool. Basically creating new connection is more expensive than acquiring a new access token, so the SqlClient driver sends a request to AAD to get a new access token and assigns it to the previously created connections inside the pool.

@David-Engel
Copy link
Contributor

What happens when using "Active Directory Default" auth? Do you actively close down the old connection pool if the token has expired?

A new access token is acquired and assigned to the old connection inside the pool. Basically creating new connection is more expensive than acquiring a new access token, so the SqlClient driver sends a request to AAD to get a new access token and assigns it to the previously created connections inside the pool.

This isn't quite right. When the token has expired and a new connection is requested that matches an existing pool, the new connection request will initiate fetching of a new token. That token will be used for a new connection to the server and saved with the pool to be used with subsequent new connections for that pool. Any old connections in the pool that used the expired token will be pruned from the pool as they are closed/disposed/freed by the application. There is no updating of tokens on connections that are already established to the server.

@David-Engel
Copy link
Contributor

I just want to add to what's happening here. AccessToken is part of the pool key, as you said. Since you have set min pool size to 10, you will get a pool of 10 connections with each unique access token. When you set a new access token, you get a new pool. The old pool is not related to the new one, so there is no "replacement" of the connections from one to the other. It's an unfortunate behavior, but by design. When you refresh your token, you should call ClearPool on your previous pool with the old token to make sure it gets cleaned up (since you have set a min pool size).

@mamort
Copy link
Author

mamort commented Aug 21, 2023

Thanks for the clarifications. It seems it would be ideal to "warm up" the new pool before the old token expires. I have now added logic to our app that does this since we typically get a new token a few minutes before the old one expire. Then, 1 minute before the old token expire we switch to using the new warmed up pool and call SqlConnection.ClearPool() on a connection with the old token. Seems to work.

This should be better documented, especially the unfortunate behavior with the combination of setting the access token on the SqlConnection + configuring MinPoolSize > 0. It would have been really nice to be able to set a Azure.Core.AccessToken on the SqlConnection instead of a string token. Then the pooling would be able to know when the token would expire and the logic I implemented above could be baked into the SqlClient-library.

@David-Engel
Copy link
Contributor

It would have been really nice to be able to set a Azure.Core.AccessToken on the SqlConnection instead of a string token.

@mamort You might be pleasantly surprised to know almost exactly that is currently available in the 5.2 preview 3 release (see #1260 for a code example). 😃

@mamort
Copy link
Author

mamort commented Aug 22, 2023

That sounds perfect! 😃

@JRahnama
Copy link
Member

@mamort I am closing the issue as resolved. Feel free to reopen the issue or create a new issue if you think there is more to be done here.

SqlClient Triage Board automation moved this from Needs Investigation to Closed Aug 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
By Design By design
Projects
Development

No branches or pull requests

3 participants