Options for connection pool COMException handling

[lmnash]

Type

[ ] Bug report. [x] Feature request.

Current Behavior

We are looking to move a large existing application over to use Insight. Due to the size and general horribleness of the existing DAL this is going to have to be a piecemeal transition and we will need to make production releases with some of the DAL using Insight and some using the existing DAL code. The brief is that we need to ensure no functional changes with this transition.

The existing DAL uses a custom wrapper to SQLConnection. Most of the behaviour in that is already covered by Insight, but our concern is how to replicate some custom handling of connection pool errors. The existing code looks like this:

            try
            {
                if (ConnectionState.Open != MyConnection.State)
                {
                    MyConnection.Open();
                }
            }
            catch (System.Runtime.InteropServices.COMException)
            {
                // if we have a comexception the most likely cause is that some thread has tried to reuse the handle in error
                // in order to allow processing to continue we need to clear the current connection pool any connection
                // currently in use will be marked for dispose after it closes and restart.
                Thread.Sleep(300);
                string conn = MyConnection.ConnectionString;
                using (SqlConnection sqlConn = new SqlConnection(conn))
                {
                    SqlConnection.ClearPool(sqlConn);
                }
                Initialize(conn);
                MyConnection.Open();
            }

I believe it was introduced to handle occasional errors as described here: https://social.msdn.microsoft.com/Forums/en-US/b5b7a179-3737-4380-b6cf-843f3e71b317/connection-pooling-randomly-throwing-com-exceptions?forum=adodotnetdataproviders

We are working through and checking all IntPtr use in the application, but its a big legacy application and its going to be tough to ensure we've eliminated the issue. I really need a way to implement this catch and check within Insight.

Expected behavior

Is there any way we can reproduce this behaviour within Insight? Ideally we'd like to still be able to use auto open and close but that is not essential. I wondered about using ReliableConnection and implementing a custom IRetryStrategy. I guess the other option would be to create and open the connection ourselves and then use Insight against that existing connection, but I'd prefer to avoid that if possible.

Steps to Reproduce

N/A

Any other comment

N/A

[jonwagner]

Given the comment in your catch statement, I would bet that the code was introduced to patch for a self-inflicted cross-thread data management issue. For example, someone put connections in a variable that persists across threads. It's difficult to tell, because the original author didn't check for any particular COM error.

Also, you're catching a COM exception, but your connection is a SqlConnection. I'm pretty sure that any recent version of SqlConnection doesn't use COM under the covers.

Finally, my guess is that the new code isn't likely to actually share any connections with the legacy code. (Definitely make sure that it doesn't, if possible.)

So, my :beer: bet is that you don't need it at all.

If you feel like you need to trap and manage this condition, implement a RetryStrategy where you override IsTransientException to detect the condition, and do the reset in OnRetrying.

If you can't reproduce the issue at the moment, I think the best strategy would be:

1. Ensure that all of your new DAL code uses a factory to create the connection.
2. Ignore the issue for now.
3. If you can reproduce the issue, then you can swap out the connection construction in the factory and use a wrapped connection.

[lmnash]

Thanks for the quick and helpful response.

Yes, self-inflicted cross-thread data management issues probably were present (someone was in love with singletons) and they should have all been refactored out now.

I'll see if I can confirm that it wouldn't throw COMException in this scenario anymore anyway, as if that is correct that should prove we don't have the issue anymore and don't need to handle it.

[jonwagner]

Closing out this issue. Feel free to reopen if you find some issues.