Skip to content
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

Stop using std::map since cleanup races with the finalizer during shutdown. #38375

Merged
merged 1 commit into from
Jun 29, 2020
Merged

Stop using std::map since cleanup races with the finalizer during shutdown. #38375

merged 1 commit into from
Jun 29, 2020
+68 −7

Conversation

AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Jun 25, 2020 •
edited
Loading

@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 5.0.0 milestone Jun 25, 2020
@AaronRobinsonMSFT AaronRobinsonMSFT changed the title Stop using std::map since it races with the finalizer during shutdown. Stop using std::map since cleanup races with the finalizer during shutdown. Jun 25, 2020
@AndyAyersMS
Copy link
Member

Wondering if this is sufficient, or the if the entire EventSink needs to be allocated somewhere other than the CRT heap?

@AaronRobinsonMSFT
Copy link
Member Author

Wondering if this is sufficient, or the if the entire EventSink needs to be allocated somewhere other than the CRT heap?

Boo. That does make sense. Not thrilled about changing that, but I can't argue with the logic.

elinor-fung
elinor-fung reviewed Jun 25, 2020
View reviewed changes
src/coreclr/tests/src/Interop/COM/NativeClients/Events/EventTests.cpp
@@ -16,28 +16,89 @@
#define THROW_IF_FAILED(exp) { hr = exp; if (FAILED(hr)) { ::printf("FAILURE: 0x%08x = %s\n", hr, #exp); throw hr; } }
#define THROW_FAIL_IF_FALSE(exp) { if (!(exp)) { ::printf("FALSE: %s\n", #exp); throw E_FAIL; } }

#include <map>
Copy link
Member

Choose a reason for hiding this comment

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

Is it just the debug iterators causing issues? Can we set _ITERATOR_DEBUG_LEVEL?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly. However @AndyAyersMS statement above about the state of the CRT being the real issue is why we may want to just remove its usage entirely.

@AaronRobinsonMSFT
Copy link
Member Author

After looking at this again I think not updating the new alloc for the object is unlikely to impact this. I will of course reconsider if we observe a failure in practice, but then we will need to reconsider all native COM server tests.

I think the current issue addresses the far more complicated issue involving the way STL collections perform validation.

@AaronRobinsonMSFT
Copy link
Member Author

Any other feedback @elinor-fung or @jkoritzinsky ?

@AaronRobinsonMSFT AaronRobinsonMSFT mentioned this pull request Jun 29, 2020
Closed
@AaronRobinsonMSFT AaronRobinsonMSFT merged commit 826b6f0 into dotnet:master Jun 29, 2020
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the fix_shutdown_race branch June 29, 2020 18:16
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@elinor-fung elinor-fung

Assignees
No one assigned
Labels
area-Interop-coreclr
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.

Test failure: Interop\\COM\\NativeClients\\Events\\Events.cmd
3 participants
@AaronRobinsonMSFT @AndyAyersMS @elinor-fung