/azp run

Codecov Report

Attention: Patch coverage is26.37615% with321 lines in your changes missing coverage. Please review.

Project coverage is 65.92%. Comparing base(0be2756) to head(644ac85).
Report is 17 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (0be2756) and HEAD (644ac85). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (0be2756)	HEAD (644ac85)
addons	1	0

@@            Coverage Diff             @@##             main    #3179      +/-   ##==========================================- Coverage   72.81%   65.92%   -6.90%==========================================  Files         282      279       -3       Lines       59112    59109       -3     ==========================================- Hits        43045    38967    -4078- Misses      16067    20142    +4075

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report?Share it here.

Thanks. The changes at the moment are no-ops, there's no behaviour change. An OutOfMemoryException without CERs means that the CLR's in an undefined state anyway, a StackOverflowException results in the process being killed and netcore doesn't throw ThreadAbortExceptions at all.

What about wrapping every call to BestEffortCleanup in#if NETFRAMEWORK? I hadn't originally wanted to do that, but that was purely cosmetic - I didn't want to leave conditional compilation scattered across the codebase. It'd be a little clearer than having a stub method though.

... there's no behaviour change...

I think the only instances where was most concerned about behavior change was when the new catches were added when there wasn't a catch-all exception handler. All the other instances are fine since they basically do the same thing as the catch-all handler.

My understanding, too, is that we really only have the CERs in place to handle situations where SqlClient is being called from SQL Server CLR. The theory being that the CER is there to protect SQL Server from crashing if something goes wrong with SqlClient. Outside of those situations, I don't care as much about cleanup behavior. I suppose my primary issue is whether we'll try to clean things up in netcore that were not being cleaned up before.

What about wrapping every call to BestEffortCleanup in#if NETFRAMEWORK?

Yeah.... that's what I had been doing for the merge I've been working on for SqlTransaction. It's messy as all heck, but it's generally clear that the code is being used or not. I've even been wrapping the extra catch blocks in the same#if. One thing I've done (that nobody's complained or applauded yet) is keep the#if tabbed over the same as the code it's wrapping. It makes it a bit less clumsy.

One experiment I was trying (and this prompted removing the resilience sections) was to use an inline function to implement the meat and potatoes of the method and either call it directly (in netcore) or pass it into a method that wraps it with appropriate CES handling. It appears that by using inline functions, it's only marginally less performant, but cleans up the code a lot. Something like:

publicvoidMyMethod(){voidDoTheStuff(){// stuff}  #ifNETFRAMEWORKcallWithCes(DoTheStuff)  #elseDoTheStuff();  #endif}

of course what would be really nice is if we could just get rid of CERs...

I think the only instances where was most concerned about behavior change was when the new catches were added when there wasn't a catch-all exception handler.

I understand. These three exceptions fall into two groups though:

1. Never thrown in netcore (which means the catch is unreachable and any change in behaviour is a moot point)
2. Thrown where the internal state of the netcore CLR is in the process of terminating as a result of a lack of memory (which kills the connection anyway)

Yeah.... that's what I had been doing for the merge I've been working on for SqlTransaction. It's messy as all heck, but it's generally clear that the code is being used or not. I've even been wrapping the extra catch blocks in the same#if.

I'll do that, thanks. I think BestEffortCleanup is the best place to draw the line, since its body relies on details of TdsParserStateObject which can't be merged and the catch blocks are functionally no-ops.

/azp run