Audit Best-effort Cleanup Code

[benrr101]

Background

As part of the merge project, we've encountered a load of situations where constrained execution region (CER) code exists in netfx but not in netcore. CER code consists (in most cases) of a try/catch block that is prepended with a PrepareConstrainedRegion call, exception catches for OutOfMemoryException, StackOverflowException, and ThreadAbortException. These regions have been a thorn in my side since there's no good way to conditionally include them in netfx and exclude them in netcore. The options that were considered include:

• #if blocks around the try/catch code
  • Creates an absolute mess in the code since indenting doesn't obey rules in netcore vs netfx.
• Just #if block around the PrepareConstrainedRegion call, but add the try/catch to netcore
  • Adds exception handlers to netcore that implies they will work, but will literally never work (ie, ThreadAbortException)
• Introduce a helper (as per Merge | SqlCommand Prepare/Unprepare #3514) and conditionally use it in netfx
  • Doesn't obey the rules of CER
  • This uncovered that CER is best effort and we are using code all over the place that does not follow the rules to observe in order for CER to work

Thus, we have the question of - what benefit does CER code serve? The answer we came up with was that CER code exists to prevent SQL Server from crashing if a SQLCLR code encounters an unrecoverable error.

... But, if CER is only best effort and we aren't following any of the rules for how it should be used, should we just obliterate it? This is where the argument happens (see #3535). [WellYesButActuallyNo.gif]

The idea of the CER code, outside the scope of preventing SQL Server from crashing, is to indicate that a connection has gone into a bad state and should not be returned to the pool (or at the very least not returned from the pool when a connection is opened). In this way, the exception handling code is beneficial in both netfx and netcore. So, should we move the CER code into netcore? Probably.

The Real Problem

We want to make our library resilient and not returning bad objects to users. But, just moving the CER code into netcore doesn't solve that problem. For one, ThreadAbortExceptions will never be thrown in netcore, so we shouldn't handle them in netcore code. Additionally, and more importantly, we handle these exceptions in really inconsistent places. Some are handled in lower levels, some are handled in higher levels, some aren't handled at all.

Goal

We need to look at our codebase and identify the places where these exceptions should be handled, and handle them in all the places they should be.

[paulmedynski]

We should consider whether any changes we make (removal of CER and/or addition of new checks) should be guarded behind feature flag. There may be subtle behaviour differences we need to gate.