Skip to content

Remove locks from COM events delegate management. #75863

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Sep 22, 2022
Merged

Remove locks from COM events delegate management. #75863

merged 9 commits into from
Sep 22, 2022

Conversation

AaronRobinsonMSFT
Copy link
Member

This removes locks and instead assumes the collection is immutable.

Fixes #73754

@AaronRobinsonMSFT
Remove locks from COM events delegate management.
2a5c722 
This removes locks and instead assumes the collection is immutable.
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 8.0.0 milestone Sep 19, 2022
@AaronRobinsonMSFT
Copy link
Member Author

/cc @dotnet/interop-contrib

@AaronRobinsonMSFT
Copy link
Member Author

@stephentoub The CI is green. Any other thoughts?

@jkotas
Copy link
Member

jkotas commented Sep 21, 2022

Use array instead of List
Revert "Use array instead of List"

Was there an actual problem or you did not like the result? (It looks better to me with arrays instead of List.)

@AaronRobinsonMSFT
Copy link
Member Author

Use array instead of List
Revert "Use array instead of List"

Was there an actual problem or you did not like the result? (It looks better to me with arrays instead of List.)

It was an option. However, I needed to create a RemoveAll local with annoying logic so deferred to just using List<T>. Either works and appear correct, other than the "Remove" logic you pointed out above. Let me see about simplifying it further.

@AaronRobinsonMSFT
Revert "Revert "Use array instead of List<T>""
93b3891 
This reverts commit aa3dddd.
@AaronRobinsonMSFT
Remove usage of Delegate.Combine. Upon deeper inspection there
4ebac6d 
doesn't seem to be any value to using that mechanism. An added
bonus is that it simplifies the logic.
@AaronRobinsonMSFT
Copy link
Member Author

@jkotas @stephentoub I switched back to using arrays, let me know if you prefer List<T>. I also removed the Delegate.Combine logic. My biggest concern with changing that related to threading since this is a COM code path. Looking through it, I no longer have any concerns so it was removed.

stephentoub
stephentoub reviewed Sep 21, 2022
View reviewed changes
@AaronRobinsonMSFT AaronRobinsonMSFT merged commit e4d115a into dotnet:main Sep 22, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the runtime_73754 branch September 22, 2022 19:28
@AaronRobinsonMSFT
Copy link
Member Author

/backport to release/7.0

@github-actions
Copy link
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3108191020

@AaronRobinsonMSFT
Copy link
Member Author

/backport to release/6.0

@github-actions
Copy link
Contributor

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

This was referenced Sep 22, 2022
Merged
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Oct 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@davidfowl davidfowl davidfowl left review comments

@jkoritzinsky jkoritzinsky jkoritzinsky left review comments

@jkotas jkotas jkotas left review comments

@stephentoub stephentoub stephentoub approved these changes

Assignees

@AaronRobinsonMSFT AaronRobinsonMSFT

Labels
area-Interop-coreclr
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

COM event with recursion .net 6 problem
5 participants
@AaronRobinsonMSFT @jkotas @davidfowl @jkoritzinsky @stephentoub