Skip to content

Remove "Host Breakable" lock concept #116611

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 13, 2025

Conversation

jkoritzinsky
Copy link
Member

This seems to have only been used by SQLCLR's hosting model (in particular for SQL Server's deadlock detection mechanism). Remove it now that there is no way for the host to break a lock.

This seems to have only been used by SQLCLR's hosting model (in particular for SQL Server's deadlock detection mechanism). Remove it now that there is no way for the host to break a lock.
@Copilot Copilot AI review requested due to automatic review settings June 12, 2025 23:12
@jkoritzinsky jkoritzinsky requested review from jkotas and removed request for Copilot June 12, 2025 23:12
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR removes the obsolete "Host Breakable" lock concept that was only used by SQLCLR’s hosting model for deadlock detection. The changes include renaming internal debug lock counters, removing CRST_HOST_BREAKABLE flags from lock initializations, and cleaning up associated macros and fields.

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/coreclr/vm/threads.h Renamed debug lock counter and its associated methods to remove host breakable terminology.
src/coreclr/vm/threads.cpp Updated constructor initialization to reflect the new lock counter naming.
src/coreclr/vm/pendingload.cpp Removed CRST_HOST_BREAKABLE flag from lock initialization.
src/coreclr/vm/listlock.h Eliminated m_fHostBreakable member and related methods.
src/coreclr/vm/excep.cpp Adjusted lock count manipulations to use the new naming.
src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.h Removed CRST_HOST_BREAKABLE flag during lock initialization.
src/coreclr/vm/dispatchinfo.cpp Updated lock initialization by removing the host breakable flag.
src/coreclr/vm/crst.cpp Cleaned up debug macro calls and lock count adjustments in light of the removal.
src/coreclr/vm/comcallablewrapper.cpp Updated crst initialization to omit CRST_HOST_BREAKABLE.
src/coreclr/vm/callhelpers.cpp Assert now verifies the updated lock state using HasLock().
src/coreclr/vm/appdomain.cpp Changed several lock initializations, removing the host breakable flag.
src/coreclr/pal/prebuilt/inc/clrinternal.h Removed the definition of CRST_HOST_BREAKABLE.
src/coreclr/inc/contract.h Removed host breakable related debug macros.
src/coreclr/inc/clrinternal.idl Commented out CRST_HOST_BREAKABLE in the IDL definitions.
Comments suppressed due to low confidence (6)

src/coreclr/vm/threads.h:3478

  • Renaming m_dwUnbreakableLockCount to m_dwLockCount is consistent with the removal of host breakable locks. Please ensure that any associated comments or documentation are updated to reflect this change.
DWORD m_dwLockCount;

src/coreclr/vm/pendingload.cpp:50

  • The removal of the CRST_HOST_BREAKABLE flag in lock initialization is appropriate given the updated design. Confirm that all related usages now rely solely on CRST_UNSAFE_SAMELEVEL.
CrstFlags(CRST_HOST_BREAKABLE|CRST_UNSAFE_SAMELEVEL));

src/coreclr/vm/listlock.h:241

  • The removal of the m_fHostBreakable field and its usage is aligned with the removal of host breakable logic. Please verify that no dependent code expects this property.
BOOL m_fHostBreakable;        // Lock can be broken by a host for deadlock detection

src/coreclr/vm/crst.cpp:424

  • The removal of the HOST_BREAKABLE_CRST_TAKEN branch and use of EE_LOCK_TAKEN is correct now. Ensure that the debug state tracking remains consistent after this change.
EE_LOCK_TAKEN(this);

src/coreclr/vm/appdomain.cpp:1682

  • Replacing the CRST_HOST_BREAKABLE flag with CRST_DEFAULT for m_FileLoadLock should be reviewed to ensure that the locking semantics are maintained as expected in the new design.
m_FileLoadLock.Init(CrstAssemblyLoader, CrstFlags(CRST_HOST_BREAKABLE), TRUE);

src/coreclr/inc/clrinternal.idl:61

  • Commenting out CRST_HOST_BREAKABLE in the IDL is consistent with its removal. Confirm that any external clients relying on this definition are updated accordingly.
CRST_HOST_BREAKABLE   = 0x20, // This lock is held while running managed code.  It can be terminated by a host.

Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@jkoritzinsky jkoritzinsky enabled auto-merge (squash) June 12, 2025 23:30
@jkoritzinsky
Copy link
Member Author

/ba-g host failure looks unrelated

@jkoritzinsky jkoritzinsky merged commit 1dd1035 into dotnet:main Jun 13, 2025
94 of 97 checks passed
@jkoritzinsky jkoritzinsky deleted the no-hostbreaking-crst branch June 13, 2025 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants