Skip to content

Cleanup/Merge | Remove Reliability Section#3042

Closed
benrr101 wants to merge 3 commits intodotnet:mainfrom
benrr101:merge/reliabilitysection-delete
Closed

Cleanup/Merge | Remove Reliability Section#3042
benrr101 wants to merge 3 commits intodotnet:mainfrom
benrr101:merge/reliabilitysection-delete

Conversation

@benrr101
Copy link
Copy Markdown
Contributor

@benrr101 benrr101 commented Nov 26, 2024

Description: After some discussion with @saurabh500 we came to the conclusion that it's just not beneficial to have the ReliabilitySection code in the netfx project. It is only included in debug builds (which we don't even use for CI builds anyhow), and it makes massive clutter in the code base. Although I've experimented with some ideas to leave it in place but cleanup the code, simply removing it is probably the easiest solution.

Testing: Project still builds without it and since it provides no functionality, it won't break anything to remove it.

@benrr101 benrr101 added Code Health 💊 Issues/PRs that are targeted to source code quality improvements. Common Project 🚮 Things that relate to the common project project labels Nov 26, 2024
@edwardneal
Copy link
Copy Markdown
Contributor

edwardneal commented Nov 26, 2024

This looks good to me. Are the TdsParser.ReliabilitySection struct definitions due to be removed in a future PR?

@MichelZ
Copy link
Copy Markdown
Contributor

MichelZ commented Nov 27, 2024 •
edited
Loading

These can be deleted as well in netcore SqlDataReader:

SqlClient/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlDataReader.cs

Lines 264 to 271 in fca9c27

#if NETFRAMEWORK && DEBUG
TdsParser.ReliabilitySection tdsReliabilitySection = new TdsParser.ReliabilitySection();
RuntimeHelpers.PrepareConstrainedRegions();
try
{
tdsReliabilitySection.Start();
#else

(and many others in there)

As well as

SqlClient/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.netfx.cs

Lines 188 to 194 in fca9c27

#if DEBUG
TdsParser.ReliabilitySection tdsReliabilitySection = new TdsParser.ReliabilitySection();
RuntimeHelpers.PrepareConstrainedRegions();
try
{
tdsReliabilitySection.Start();
#endif //DEBUG

(and others in there)

and of course the defitinition

SqlClient/src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.netfx.cs

Lines 12 to 92 in fca9c27

// ReliabilitySection Usage:
//
// #if DEBUG
// TdsParser.ReliabilitySection tdsReliabilitySection = new TdsParser.ReliabilitySection();
//
// RuntimeHelpers.PrepareConstrainedRegions();
// try {
// tdsReliabilitySection.Start();
// #else
// {
// #endif //DEBUG
//
// // code that requires reliability
//
// }
// #if DEBUG
// finally {
// tdsReliabilitySection.Stop();
// }
// #endif //DEBUG
internal struct ReliabilitySection
{
#if DEBUG
// do not allocate TLS data in RETAIL bits
[ThreadStatic]
private static int s_reliabilityCount; // initialized to 0 by CLR
private bool m_started; // initialized to false (not started) by CLR
#endif //DEBUG
[Conditional("DEBUG")]
internal void Start()
{
#if DEBUG
Debug.Assert(!m_started);
RuntimeHelpers.PrepareConstrainedRegions();
try
{
}
finally
{
++s_reliabilityCount;
m_started = true;
}
#endif //DEBUG
}
[Conditional("DEBUG")]
internal void Stop()
{
#if DEBUG
// cannot assert m_started - ThreadAbortException can be raised before Start is called
if (m_started)
{
Debug.Assert(s_reliabilityCount > 0);
RuntimeHelpers.PrepareConstrainedRegions();
try
{
}
finally
{
--s_reliabilityCount;
m_started = false;
}
}
#endif //DEBUG
}
// you need to setup for a thread abort somewhere before you call this method
[Conditional("DEBUG")]
internal static void Assert(string message)
{
#if DEBUG
Debug.Assert(s_reliabilityCount > 0, message);
#endif //DEBUG
}
}

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 27, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 62.36453% with 382 lines in your changes missing coverage. Please review.

Project coverage is 72.66%. Comparing base (46e8714) to head (1c61d33).
Report is 29 commits behind head on main.

Files with missing lines Patch % Lines
...core/src/Microsoft/Data/SqlClient/SqlDataReader.cs 56.84% 123 Missing ⚠️
...etfx/src/Microsoft/Data/SqlClient/SqlDataReader.cs 57.04% 122 Missing ⚠️
...t/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs 62.10% 83 Missing ⚠️
...icrosoft/Data/SqlClient/SqlDelegatedTransaction.cs 59.32% 24 Missing ⚠️
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs 43.75% 18 Missing ⚠️
...etfx/src/Microsoft/Data/SqlClient/SqlConnection.cs 74.28% 9 Missing ⚠️
...crosoft/Data/SqlClient/SqlInternalConnectionTds.cs 91.66% 2 Missing ⚠️
...fx/src/Microsoft/Data/SqlClient/TdsParser.netfx.cs 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #3042   +/-   ##
=======================================
  Coverage   72.66%   72.66%           
=======================================
  Files         283      283           
  Lines       58975    58909   -66     
=======================================
- Hits        42853    42806   -47     
+ Misses      16122    16103   -19
Flag Coverage Δ
addons 92.58% <ø> (ø)
netcore 75.44% <60.16%> (+<0.01%) ⬆️
netfx 71.12% <64.52%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@benrr101 benrr101 force-pushed the merge/reliabilitysection-delete branch from 73caecc to c099dab Compare November 27, 2024 23:19
@cheenamalhotra cheenamalhotra added this to the 7.0-preview1 milestone Dec 3, 2024
@MichelZ
Copy link
Copy Markdown
Contributor

MichelZ commented Dec 17, 2024

@cheenamalhotra @mdaigle Can this be merged? That would help with #2953

@MichelZ MichelZ mentioned this pull request Dec 17, 2024
Closed
@edwardneal edwardneal mentioned this pull request Jan 2, 2025
Closed
@mdaigle mdaigle mentioned this pull request Feb 13, 2025
Merged
@benrr101 benrr101 mentioned this pull request Feb 19, 2025
Merged
@benrr101 benrr101 closed this Feb 19, 2025
@benrr101 benrr101 deleted the merge/reliabilitysection-delete branch February 19, 2025 19:50
@cheenamalhotra cheenamalhotra removed this from the 6.1-preview1 milestone Mar 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mdaigle mdaigle mdaigle approved these changes

@paulmedynski paulmedynski paulmedynski approved these changes

@saurabh500 saurabh500 Awaiting requested review from saurabh500

@cheenamalhotra cheenamalhotra Awaiting requested review from cheenamalhotra

@David-Engel David-Engel Awaiting requested review from David-Engel

@samsharma2700 samsharma2700 Awaiting requested review from samsharma2700

+1 more reviewer

@edwardneal edwardneal edwardneal left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Code Health 💊 Issues/PRs that are targeted to source code quality improvements. Common Project 🚮 Things that relate to the common project project

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@benrr101 @edwardneal @MichelZ @mdaigle @paulmedynski @cheenamalhotra