Skip to content

Check if External Object Context is still active after a possible GC - #59285

Merged
AaronRobinsonMSFT merged 7 commits into
dotnet:mainfrom
AaronRobinsonMSFT:runtime59072
Sep 21, 2021
Merged

Check if External Object Context is still active after a possible GC#59285
AaronRobinsonMSFT merged 7 commits into
dotnet:mainfrom
AaronRobinsonMSFT:runtime59072

Conversation

@AaronRobinsonMSFT

@AaronRobinsonMSFTAaronRobinsonMSFT commented Sep 17, 2021

Copy link
Copy Markdown
Member

Comments in this code detail the logic and reasons for failure.

This should be considered for .NET 6.

Fixes#59072

/cc @davidwrighton@jkoritzinsky@elinor-fung

@AaronRobinsonMSFTAaronRobinsonMSFT added this to the 7.0.0 milestone Sep 17, 2021
@AaronRobinsonMSFT

AaronRobinsonMSFT commented Sep 18, 2021

Copy link
Copy Markdown
MemberAuthor

/backport to release/6.0

@github-actions

github-actionsBot commented Sep 18, 2021

Copy link
Copy Markdown
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1247477775

@github-actionsgithub-actionsBot mentioned this pull request Sep 18, 2021
Closed
elinor-fung
elinor-fung approved these changes Sep 20, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/interoplibinterface_comwrappers.cpp Outdated
Comment threadsrc/coreclr/vm/interoplibinterface_comwrappers.cpp Outdated
@dotnetdotnet deleted a comment from elinor-fungSep 20, 2021
@dotnetdotnet deleted a comment from elinor-fungSep 20, 2021
AaronRobinsonMSFT
AaronRobinsonMSFT commented Sep 20, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/interoplibinterface_comwrappers.cpp Outdated
@AaronRobinsonMSFT@elinor-fung
Apply suggestions from code review
bc4b829
Co-authored-by: Elinor Fung <elfung@microsoft.com>
@AaronRobinsonMSFT@elinor-fung
Apply suggestions from code review
e820da6
Co-authored-by: Elinor Fung <elfung@microsoft.com>
@davidwrighton

davidwrighton commented Sep 21, 2021

Copy link
Copy Markdown
Member
 gc.arrRef = (PTRARRAYREF)AllocateObjectArray((DWORD)localList.Size(), g_pObjectClass);

This change relies on this AllocateObjectArray call not triggering multiple GCs. While I think that such behavior would be unusual, I don't think its a good practice to rely on such a detail.

Refers to: src/coreclr/vm/interoplibinterface_comwrappers.cpp:355 in e820da6. [](commit_id = e820da6, deletion_comment = False)

davidwrighton
davidwrighton reviewed Sep 21, 2021
View reviewed changes
Comment threadsrc/coreclr/vm/interoplibinterface_comwrappers.cpp Outdated
davidwrighton
davidwrighton approved these changes Sep 21, 2021
View reviewed changes

@davidwrightondavidwrighton 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.

The allocation problem here now looks solved.

@AaronRobinsonMSFT
AaronRobinsonMSFT merged commit 2f68bf6 into dotnet:mainSep 21, 2021
@AaronRobinsonMSFT
AaronRobinsonMSFT deleted the runtime59072 branch September 21, 2021 15:01
AaronRobinsonMSFT added a commit that referenced this pull request Sep 21, 2021
@AaronRobinsonMSFT@elinor-fung
Check if External Object Context is still active after a possible GC (#…
dedd799
…59285)
* Handle a possible GC after allocating an array by iterating over the hash again.
Co-authored-by: Elinor Fung <elfung@microsoft.com>
@AaronRobinsonMSFT

AaronRobinsonMSFT commented Sep 21, 2021

Copy link
Copy Markdown
MemberAuthor

/backport to release/6.0-rc2

@github-actions

github-actionsBot commented Sep 21, 2021

Copy link
Copy Markdown
Contributor

Started backporting to release/6.0-rc2: https://github.com/dotnet/runtime/actions/runs/1259570246

@github-actionsgithub-actionsBot mentioned this pull request Sep 21, 2021
Merged
@ghostghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@davidwrightondavidwrightondavidwrighton approved these changes
@elinor-fungelinor-fungelinor-fung approved these changes

Assignees

No one assigned

Labels

area-Interop-coreclr

Projects

None yet

Milestone

7.0.0

3 participants

@AaronRobinsonMSFT@davidwrighton@elinor-fung