Skip to content

Replace LowLevelLock with Lock - #94991

Merged
MichalStrehovsky merged 1 commit into
mainfrom
MichalStrehovsky-patch-1
Nov 21, 2023
Merged

Replace LowLevelLock with Lock#94991
MichalStrehovsky merged 1 commit into
mainfrom
MichalStrehovsky-patch-1

Conversation

@MichalStrehovsky

@MichalStrehovskyMichalStrehovsky commented Nov 20, 2023

Copy link
Copy Markdown
Member

Context: #94873 (comment)

Cc @dotnet/ilc-contrib

@ghost

ghost commented Nov 20, 2023

Copy link
Copy Markdown

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Context: #94873 (comment)

Cc @dotnet/ilc-contrib

Author:MichalStrehovsky
Assignees:-
Labels:

area-NativeAOT-coreclr

Milestone:-

@MichalStrehovskyMichalStrehovsky mentioned this pull request Nov 20, 2023
Merged
VSadov
VSadov approved these changes Nov 20, 2023
View reviewed changes

@VSadovVSadov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@MichalStrehovsky
MichalStrehovsky merged commit 3b61ad9 into mainNov 21, 2023
@MichalStrehovsky
MichalStrehovsky deleted the MichalStrehovsky-patch-1 branch November 21, 2023 07:46
@jkotas

jkotas commented Nov 21, 2023

Copy link
Copy Markdown
Member

I do not think that this is a good change, and it should be reverted. This change can lead to reentrancy crashes if user code overrides SynchronizationContext.Wait and the override attempts to create System.RuntimeType instance (e.g. by calling `object.GetType()).

#94873 (comment) has more context.

MichalStrehovsky added a commit that referenced this pull request Nov 21, 2023
@MichalStrehovsky
Revert "Replace LowLevelLock with Lock (#94991)"
fd5d7a0
This reverts commit 3b61ad9.
@MichalStrehovskyMichalStrehovsky mentioned this pull request Nov 21, 2023
Merged
@MichalStrehovsky

MichalStrehovsky commented Nov 21, 2023

Copy link
Copy Markdown
MemberAuthor

I do not think that this is a good change, and it should be reverted. This change can lead to reentrancy crashes if user code overrides SynchronizationContext.Wait and the override attempts to create System.RuntimeType instance (e.g. by calling `object.GetType()).

#94873 (comment) has more context.

Revert in #95071.

stephentoub pushed a commit that referenced this pull request Nov 21, 2023
@MichalStrehovsky
Revert "Replace LowLevelLock with Lock (#94991)" (#95071)
1050695
This reverts commit 3b61ad9.
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Dec 22, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@VSadovVSadovVSadov approved these changes

Assignees

@MichalStrehovskyMichalStrehovsky

Labels

area-NativeAOT-coreclr

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@MichalStrehovsky@jkotas@VSadov