Skip to content

[release/5.0] Notify Reference Tracker runtime of disconnect at the right time - #58412

Merged
Anipik merged 3 commits into
dotnet:release/5.0from
AaronRobinsonMSFT:reftracker_disconnect_semantic
Sep 13, 2021
Merged

[release/5.0] Notify Reference Tracker runtime of disconnect at the right time#58412
Anipik merged 3 commits into
dotnet:release/5.0from
AaronRobinsonMSFT:reftracker_disconnect_semantic

Conversation

@AaronRobinsonMSFT

@AaronRobinsonMSFTAaronRobinsonMSFT commented Aug 31, 2021•
edited
Loading

Copy link
Copy Markdown
Member

Fixesmicrosoft/CsWinRT#840

Customer Impact

Based on misunderstanding of SyncBlock clean-up modes the indication of when an Native Object Wrapper was being collected was being done after the wrapper's Finalizer was run. However, this information must be conveyed prior to wrapper finalization and synchronously during the GC. See IReferenceTracker::DisconnectFromTrackerSource.

Workaround

There are no workarounds for this issue.

Testing

Added a tests. Have provided private binaries to C#/WinRT team for validation.

Risk

Minimal. .NET 5 is the first release of ComWrappers and this issue only impacts WinRT scenarios.

/cc @davidwrighton@jkoritzinsky@elinor-fung

@AaronRobinsonMSFT
Notify Reference Tracker runtime of disconnect at the right time
1b211db
Based on misunderstanding of SyncBlock clean-up modes the indication of
when an Native Object Wrapper was being collected was being done after
the wrapper's Finalizer was run. However, this information must be conveyed
prior to wrapper finalization and synchronously during the GC.
@AaronRobinsonMSFTAaronRobinsonMSFT added Servicing-consider Issue for next servicing release review area-Interop-coreclr labels Aug 31, 2021
jeffschwMSFT
jeffschwMSFT approved these changes Aug 31, 2021
View reviewed changes

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

Approved. Please get a code review and we can take for consideration for 5.0.x.

@jeffschwMSFTjeffschwMSFT added this to the 5.0.x milestone Aug 31, 2021
@manodasanWmanodasanW mentioned this pull request Aug 31, 2021
Closed
elinor-fung
elinor-fung approved these changes Aug 31, 2021
View reviewed changes

@elinor-fungelinor-fung 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.

My comments are minor things for when this goes to 7.0 - not so much for 5.0

Comment threadsrc/tests/Interop/COM/ComWrappers/Common.cs Outdated
Comment threadsrc/coreclr/src/interop/inc/interoplib.h Outdated
Comment threadsrc/tests/Interop/COM/ComWrappers/MockReferenceTrackerRuntime/ReferenceTrackerRuntime.cpp Outdated
@AaronRobinsonMSFT

AaronRobinsonMSFT commented Aug 31, 2021

Copy link
Copy Markdown
MemberAuthor

My comments are minor things for when this goes to 7.0 - not so much for 5.0

There is no .NET 7 PR yet, but I will fold this feedback into that when it is opened.

@AaronRobinsonMSFTAaronRobinsonMSFT mentioned this pull request Aug 31, 2021
Merged
@AaronRobinsonMSFTAaronRobinsonMSFT changed the title Notify Reference Tracker runtime of disconnect at the right time[.NET 5.0] Notify Reference Tracker runtime of disconnect at the right timeAug 31, 2021
AaronRobinsonMSFT
AaronRobinsonMSFT commented Sep 1, 2021
View reviewed changes
Comment threadsrc/tests/Interop/COM/ComWrappers/Common.cs Outdated
AaronRobinsonMSFT
AaronRobinsonMSFT commented Sep 1, 2021
View reviewed changes
Comment threadsrc/coreclr/src/vm/interoplibinterface.cpp Outdated
AaronRobinsonMSFT
AaronRobinsonMSFT commented Sep 1, 2021
View reviewed changes
Comment threadsrc/coreclr/src/interop/interoplib.cpp Outdated
AaronRobinsonMSFT
AaronRobinsonMSFT commented Sep 1, 2021
View reviewed changes
Comment threadsrc/coreclr/src/interop/interoplib.cpp Outdated
davidwrighton
davidwrighton approved these changes Sep 1, 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.

:shipit:

@leecowleecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 2, 2021
@leecowleecow modified the milestones: 5.0.x, 5.0.11Sep 2, 2021
@AaronRobinsonMSFTAaronRobinsonMSFT changed the title [.NET 5.0] Notify Reference Tracker runtime of disconnect at the right time[release/5.0] Notify Reference Tracker runtime of disconnect at the right timeSep 3, 2021
@Anipik
Anipik merged commit 7535d15 into dotnet:release/5.0Sep 13, 2021
@Marv51

Marv51 commented Sep 13, 2021

Copy link
Copy Markdown
Contributor

Is there a reason this was scheduled for 5.0.11 and not the next update? I guess 5.0.11 could be roughly expected in the middle of October, right?

@leecow

@AaronRobinsonMSFT
AaronRobinsonMSFT deleted the reftracker_disconnect_semantic branch September 13, 2021 20:58
@AaronRobinsonMSFT

AaronRobinsonMSFT commented Sep 13, 2021

Copy link
Copy Markdown
MemberAuthor

@Marv51 Yes, this is for the October update. Unfortunately we missed the 5.0.10 release snap so this was pushed to the next service release.

@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

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

Assignees

No one assigned

Labels

area-Interop-coreclrServicing-approvedApproved for servicing release

Projects

None yet

Milestone

5.0.11

Development

Successfully merging this pull request may close these issues.

8 participants

@AaronRobinsonMSFT@Marv51@jkoritzinsky@jeffschwMSFT@davidwrighton@elinor-fung@leecow@Anipik