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 | Fix possible server connection leak if an exception occurs in pooling layer #890

Merged
merged 8 commits into from
Apr 14, 2021

Conversation

cheenamalhotra
Copy link
Member

@cheenamalhotra cheenamalhotra commented Jan 26, 2021

This PR is additional fix for consequences of error Internal .NET Framework Data Provider error 6. that is thrown from DbConnectionPool.CreateObject method when something goes wrong in Pooling layer.

Continued investigations over original issue that was fixed by #637 led to figuring out that the catch blocks in this method simply set newObj to null, leaking resources and TDS connections on server, that can only be garbage collected on client when server drops them.

This causes expensive connection leaks on server, and issue exists in System.Data.SqlClient and Microsoft.Data.SqlClient (<2.0.1).
In Microsoft.Data.SqlClient we fixed this issue with PR #638 and #637, this PR is also a fix that ensures resources are cleaned properly in any similar future events.

@saurabh500
Copy link
Contributor

saurabh500 commented Jan 26, 2021

This change can be tested using the TestTdeServer and injecting transient failures
e.g.

using (TestTdsServer server = TestTdsServer.StartTestServer())

Another example of the usage of server is at

using (var server = TestTdsServer.StartServerWithQueryEngine(new DiagnosticsQueryEngine(), enableLog: enableServerLogging, methodName: methodName))

@JRahnama
Copy link
Contributor

LGTM.

@cheenamalhotra cheenamalhotra added this to the 3.0.0-preview1 milestone Feb 19, 2021
@cheenamalhotra
Copy link
Member Author

/azp run CI-BuildNPack

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Base automatically changed from master to main March 15, 2021 17:54
@cheenamalhotra
Copy link
Member Author

This change can be tested using the TestTdeServer and injecting transient failures

Hi @saurabh500 I've added the test case and I can also reproduce Internal .NET Framework Data Provider error 6. with connection leak with the new test "TransientFaultTest". You'd need to revert changes from this PR and #637 (just comment out changes) to reproduce it locally.

In order to capture the leaked connection, you'll need to add breakpoint in exception handling logic in the test and capture heap snapshot. SqlConnection instance no longer contains reference of this "Open" internal connection so it's reference cannot be captured via reflection and hence state is always Closed.

Navigating through Heap Snashot you will come across 1 instance of TdsParserStateObjectNative which has TdsParser reference and it's State is OpenLoggedIn. Since only internal connection reference is dropped in the code where I fixed in this PR, the TCP connection to server is intact and is leaked.

image

@cheenamalhotra cheenamalhotra merged commit c34e8a4 into dotnet:main Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants