Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Dont allow exceptions to emerge on the threadpool #27439

Merged
merged 3 commits into from
Feb 25, 2018
Merged

Dont allow exceptions to emerge on the threadpool #27439

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ee19a20
Dont allow exceptions to emerge on the threadpool
Feb 24, 2018
cfc6b02
Code review comments
Feb 25, 2018
48b7fe8
Check for specific exceptions
Feb 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 11 additions & 3 deletions src/System.Data.SqlClient/src/System/Data/ProviderBase/DbConnectionPool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1376,9 +1376,17 @@ private void PoolCreateRequest(object state)
{
while (NeedToReplenish)
{
// Don't specify any user options because there is no outer connection associated with the new connection
newObj = CreateObject(owningObject: null, userOptions: null, oldConnection: null);

try
{
// Don't specify any user options because there is no outer connection associated with the new connection
newObj = CreateObject(owningObject: null, userOptions: null, oldConnection: null);
}
catch(Exception)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're catching everything, you can just do catch without the Exception part.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

{
// Catch all the exceptions occuring during CreateObject so that they
// don't emerge as unhandled on the thread pool and don't crash applications
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be good to add why it's ok that we eat them... from your PR description, it sounds like there's some other backchannel through which they're passed up to the originating code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.
What happens is that CreateObject handles the exception at

corefx/src/System.Data.SqlClient/src/System/Data/ProviderBase/DbConnectionPool.cs

Line 721 in 51bdece

if (!ADP.IsCatchableExceptionType(e))

And then it sets the ErrorEvent. The caller of the connection pool is signalled on the connection pool about the ErrorEvent being set and it knows that an exception was thrown. The caller of Connection.Open() ultimately gets the same exception that they were receiving on .Net Framework. It's just that the exception doesn't escape on to the threadpool

break;
}
// We do not need to check error flag here, since we know if
// CreateObject returned null, we are in error case.
if (null != newObj)
Expand Down
23 changes: 23 additions & 0 deletions src/System.Data.SqlClient/tests/FunctionalTests/SqlConnectionBasicTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,29 @@ public void ConnectionTimeoutTestWithThread()
Console.WriteLine($"ConnectionTimeoutTestWithThread: Elapsed Time {theMax} and threshold {threshold}");
}

[Fact]
public void ExceptionsWithMinPoolSizeCanBeHandled()
{
int count = 0;
int expected = 3;
string connectionString = $"Data Source={Guid.NewGuid().ToString()};uid=random;pwd=asd;Connect Timeout=2; Min Pool Size=3";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this Connect Timeout mean this will take 2 seconds to fail? And we do it 3 times? Might want to make the test outerloop then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test will try for a max of 2 seconds. However it could fail earlier. But it will go upto 2 seconds.
Outerloop makes sense. Thanks.

while (count < expected)
{
using (SqlConnection connection = new SqlConnection(connectionString))
{
try
{
connection.Open();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this going to fail every time? If so, how about using Assert.Throws? That would make it something like:

string connectionString = ...;
for (int i = 0; i < 3; i++)
{
    using (var connection = new SqlConnection(connectionString))
    {
        Assert.Throws<WhateverExceptionType>(() => connection.Open());
    }
}

That'll also help validate that the right exception is thrown.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it will fail everytime. I will adapt the testcase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: The exception thrown is either InvalidOperationException (if the connection pool timeout is triggered) or SqlException (if the network error in connecting to invalid Data Source happens before the Connection Pool timeout) . So the specific exception type cannot be determined. I will use Assert.ThrowsAny<Exception>(() => connection.Open());

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the specific exception type cannot be determined. I will use Assert.ThrowsAny(() => connection.Open());

In that case you can do:

Exception error = Record.Exception(() => connection.Open());
Assert.True(error is InvalidOperationException || error is SqlException, $"Unexpected exception: {error}");

}
catch (Exception)
{
count++;
}
}
}
Assert.Equal(expected, count);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do end up keeping the current structure, this Assert.Equal seems unnecessary... the only way you'll get here, by construction, is if expected == count, as that's when you exit the loop.

}

public class ConnectionWorker
{
private static ManualResetEventSlim startEvent = new ManualResetEventSlim(false);
Expand Down