dotnet / dotnet/SqlClient

Audit Best-effort Cleanup Code

Open
#3,581 1 comment 2 reactions 0 assignees View on GitHub
Code Health :pill:
Dominant language
C#
Stars
989
Forks
340
Avg merge
4d 18h
Merged PRs (30d)
69

Description

## 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`](https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.runtimehelpers.prepareconstrainedregions?view=net-9.0) 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 #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, `ThreadAbortException`s 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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.