Dont allow exceptions to emerge on the threadpool

[saurabh500]

PoolCreateRequest handles creation of new connections on the thread pool. However when an exception occurs, it is bubbled up to the thread pool. This is a regression from .Net Framework and causes applications using Min Pool Size to crash.

The solution is to catch all exceptions during connection open and to swallow them. This will not change the exception that app sees from Connection Pool, as the error is bubbled up from another code path where the errorWaitHandle is set during connection Open.

The test added, makes sure that we can open 3 bad connections using MinPoolSize. Before the fix, the test would crash after the first exception.

Fixes https://github.com/dotnet/corefx/issues/14615

[saurabh500]

@dotnet-bot Test Windows x64 Debug Build please

@danmosemsft I can figure out what failed for the given build from the logs.

https://ci3.dot.net/job/dotnet_corefx/job/master/job/windows-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/8908/consoleFull

[stephentoub]

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?

[saurabh500]

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

[stephentoub]

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.

[saurabh500]

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

[saurabh500]

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());

[stephentoub]

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}");

[stephentoub]

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.

[davidfowl]

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

[saurabh500]

Agreed.

[stephentoub]

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.

[saurabh500]

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.