dotnet / SqlClient

Microsoft.Data.SqlClient provides database connectivity to SQL Server for .NET applications.
MIT License
854 stars 286 forks source link

SqlConnection.OpenAsync() may block on network i/o during connection creation #979

Open coderb opened 3 years ago

coderb commented 3 years ago

please see the stack trace below.

SqlConnection.OpenAsync() has a code path that does synchronous blocking network i/o to the sql server.

my use case is a sql server reboot during a client request. the stack below was called from a u/i thread which subsequently hung the application due to OpenAsync() doing synchronous network i/o rather than awaiting.

(version 2.1.0 netcore)

        [Managed to Native Transition]  
        Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SNINativeMethodWrapper.SNIOpenSyncEx(Microsoft.Data.SqlClient.SNINativeMethodWrapper.ConsumerInfo consumerInfo, string constring, ref System.IntPtr pConn, byte[] spnBuffer, byte[] instanceName, bool fOverrideCache, bool fSync, int timeout, bool fParallel, Microsoft.Data.SqlClient.SQLDNSInfo cachedDNSInfo) Line 405       C#
        Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SNIHandle.SNIHandle(Microsoft.Data.SqlClient.SNINativeMethodWrapper.ConsumerInfo myInfo, string serverName, byte[] spnBuffer, bool ignoreSniOpenTimeout, int timeout, out byte[] instanceName, bool flushCache, bool fSync, bool fParallel, Microsoft.Data.SqlClient.SQLDNSInfo cachedDNSInfo) Line 161   C#
        Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.TdsParserStateObjectNative.CreatePhysicalSNIHandle(string serverName, bool ignoreSniOpenTimeout, long timerExpire, out byte[] instanceName, ref byte[] spnBuffer, bool flushCache, bool async, bool fParallel, string cachedFQDN, ref Microsoft.Data.SqlClient.SQLDNSInfo pendingDNSInfo, bool isIntegratedSecurity) Line 175     C#
        Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.TdsParser.Connect(Microsoft.Data.SqlClient.ServerInfo serverInfo, Microsoft.Data.SqlClient.SqlInternalConnectionTds connHandler, bool ignoreSniOpenTimeout, long timerExpire, bool encrypt, bool trustServerCert, bool integratedSecurity, bool withFailover, Microsoft.Data.SqlClient.SqlAuthenticationMethod authType) Line 445 C#
        Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlInternalConnectionTds.AttemptOneLogin(Microsoft.Data.SqlClient.ServerInfo serverInfo, string newPassword, System.Security.SecureString newSecurePassword, bool ignoreSniOpenTimeout, Microsoft.Data.ProviderBase.TimeoutTimer timeout, bool withFailover) Line 1880    C#
        Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlInternalConnectionTds.LoginNoFailover(Microsoft.Data.SqlClient.ServerInfo serverInfo, string newPassword, System.Security.SecureString newSecurePassword, bool redirectedUserInstance, Microsoft.Data.SqlClient.SqlConnectionString connectionOptions, Microsoft.Data.SqlClient.SqlCredential credential, Microsoft.Data.ProviderBase.TimeoutTimer timeout) Line 1511  C#
        Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlInternalConnectionTds.OpenLoginEnlist(Microsoft.Data.ProviderBase.TimeoutTimer timeout, Microsoft.Data.SqlClient.SqlConnectionString connectionOptions, Microsoft.Data.SqlClient.SqlCredential credential, string newPassword, System.Security.SecureString newSecurePassword, bool redirectedUserInstance) Line 1414  C#
        Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlInternalConnectionTds.SqlInternalConnectionTds(Microsoft.Data.ProviderBase.DbConnectionPoolIdentity identity, Microsoft.Data.SqlClient.SqlConnectionString connectionOptions, Microsoft.Data.SqlClient.SqlCredential credential, object providerInfo, string newPassword, System.Security.SecureString newSecurePassword, bool redirectedUserInstance, Microsoft.Data.SqlClient.SqlConnectionString userConnectionOptions, Microsoft.Data.SqlClient.SessionData reconnectSessionData, bool applyTransientFaultHandling, string accessToken, Microsoft.Data.ProviderBase.DbConnectionPool pool) Line 507        C#
        Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlConnectionFactory.CreateConnection(Microsoft.Data.Common.DbConnectionOptions options, Microsoft.Data.Common.DbConnectionPoolKey poolKey, object poolGroupProviderInfo, Microsoft.Data.ProviderBase.DbConnectionPool pool, System.Data.Common.DbConnection owningConnection, Microsoft.Data.Common.DbConnectionOptions userOptions) Line 133    C#
        Microsoft.Data.SqlClient.dll!Microsoft.Data.ProviderBase.DbConnectionFactory.CreatePooledConnection(Microsoft.Data.ProviderBase.DbConnectionPool pool, System.Data.Common.DbConnection owningObject, Microsoft.Data.Common.DbConnectionOptions options, Microsoft.Data.Common.DbConnectionPoolKey poolKey, Microsoft.Data.Common.DbConnectionOptions userOptions) Line 138      C#
        Microsoft.Data.SqlClient.dll!Microsoft.Data.ProviderBase.DbConnectionPool.CreateObject(System.Data.Common.DbConnection owningObject, Microsoft.Data.Common.DbConnectionOptions userOptions, Microsoft.Data.ProviderBase.DbConnectionInternal oldConnection) Line 726    C#
        Microsoft.Data.SqlClient.dll!Microsoft.Data.ProviderBase.DbConnectionPool.UserCreateRequest(System.Data.Common.DbConnection owningObject, Microsoft.Data.Common.DbConnectionOptions userOptions, Microsoft.Data.ProviderBase.DbConnectionInternal oldConnection) Line 1737      C#
        Microsoft.Data.SqlClient.dll!Microsoft.Data.ProviderBase.DbConnectionPool.TryGetConnection(System.Data.Common.DbConnection owningObject, uint waitForMultipleObjectsTimeout, bool allowCreate, bool onlyOneCheckConnection, Microsoft.Data.Common.DbConnectionOptions userOptions, out Microsoft.Data.ProviderBase.DbConnectionInternal connection) Line 1256   C#
        Microsoft.Data.SqlClient.dll!Microsoft.Data.ProviderBase.DbConnectionPool.TryGetConnection(System.Data.Common.DbConnection owningObject, System.Threading.Tasks.TaskCompletionSource<Microsoft.Data.ProviderBase.DbConnectionInternal> retry, Microsoft.Data.Common.DbConnectionOptions userOptions, out Microsoft.Data.ProviderBase.DbConnectionInternal connection) Line 1114 C#
        Microsoft.Data.SqlClient.dll!Microsoft.Data.ProviderBase.DbConnectionFactory.TryGetConnection(System.Data.Common.DbConnection owningConnection, System.Threading.Tasks.TaskCompletionSource<Microsoft.Data.ProviderBase.DbConnectionInternal> retry, Microsoft.Data.Common.DbConnectionOptions userOptions, Microsoft.Data.ProviderBase.DbConnectionInternal oldConnection, out Microsoft.Data.ProviderBase.DbConnectionInternal connection) Line 121   C#
>       Microsoft.Data.SqlClient.dll!Microsoft.Data.ProviderBase.DbConnectionInternal.TryOpenConnectionInternal(System.Data.Common.DbConnection outerConnection, Microsoft.Data.ProviderBase.DbConnectionFactory connectionFactory, System.Threading.Tasks.TaskCompletionSource<Microsoft.Data.ProviderBase.DbConnectionInternal> retry, Microsoft.Data.Common.DbConnectionOptions userOptions) Line 338        C#
        Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlConnection.TryOpen(System.Threading.Tasks.TaskCompletionSource<Microsoft.Data.ProviderBase.DbConnectionInternal> retry, Microsoft.Data.SqlClient.SqlConnectionOverrides overrides) Line 1721   C#
        Microsoft.Data.SqlClient.dll!Microsoft.Data.SqlClient.SqlConnection.OpenAsync(System.Threading.CancellationToken cancellationToken) Line 1474   C#
        ... my code
cheenamalhotra commented 3 years ago

Hi @coderb

Thanks for opening up this issue. This is a known design issue as driver internals are still synchronously designed and not everything supports awaiting yet. It's also a very heavy activity and requires major rework in the driver.

We'll have to see if this receives more community interest and shall be prioritized accordingly.

coderb commented 3 years ago

Would it be better to implement OpenAsync() as return Task.Run(connection.Open) in the interim which would unblock the caller thread? This way users can code to the async api without workarounds and the application code wouldn't need to be updated when the library is fully async.

madelson commented 3 years ago

@coderb I'm personally not a fan of having async methods needlessly do that, since it just adds overhead for the common case. 100% agree that OpenAsync() should be implemented with as much asynchrony as possible, though.

We'll have to see if this receives more community interest and shall be prioritized accordingly.

@cheenamalhotra my worry is that this issue is going to be pretty invisible to most unless it causes a major problem. I certainly had assumed that OpenAsync() was fully asynchronous at this point, but never thought to dig in and check.

roji commented 3 years ago

Would it be better to implement OpenAsync() as return Task.Run(connection.Open) in the interim which would unblock the caller thread?

This could trigger TP starvation issues, as TP threads synchronously block on connection.Open, and so none are available to execute async callbacks.

rducom commented 3 years ago

We experienced a thread pool starvation issue today with this, while having connection pooling disabled. 28000 treads created for 28000 SQL calls in 12min30s. Hugues process pauses. Reactivating SqlConnection pooling fixes the issue.

1ms threads are created just to initiate the connection.

image

FYI, we disabled pooling because of a process needing to make a few calls on 2000 distinct databases, with specific connection string for each.

JRahnama commented 1 year ago

Closing the issue as it is by design. Feel free to comment here or open a new issue.

roji commented 1 year ago

@JRahnama are you saying it's by-design for OpenAsync to perform synchronous I/O and block the thread? OpenAsync really should never block the thread on I/O.

JRahnama commented 1 year ago

@roji I mixed this issue with another one I was looking at. Thanks for catching my mistake.

Reopening the issue as it was closed by mistake.

marcinsmialek commented 1 year ago

Can we remove the "By Design" label, so this issue could get more attention?

Farenheith commented 8 months ago

I support this issue as it can remove a little bottleneck specially when we're talking about aspnet core. Due to the fact asptnet core controls multiple requests mainly through Tasks, not Threads, the fact OpenAsync actually runs syncrhonously under the hood makes requests on the same Thread to be queued until each one acquires a Connection from the pool.

Of course, if the connections are already created, this is not an issue at all, but let's suppose the minimum pool is 1, while the maximum is 100 (default config), the api is almost idle and a sudden heavy load reaches the API. In this scenario, multiple connection we'll be created, causing an undesired enqueing of OpenAsync calls on the Thread, even though SQL Server may easily handle multiple connections being stablished.

A similar issue may happen in any application focused on Tasks, like a custom one to consume a SQS Queue, for example.