Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

brianrob
Copy link
Member

Port the fix contained in #14123 to release/2.0.

* Fix a crash that occurs when a provider is registered after the configuration object has been destroyed.

* Code review feedback.
@brianrob brianrob requested a review from nategraf November 21, 2017 22:18
@brianrob
Copy link
Member Author

@nategraf, this fix is just for the crash. I had to modify it a bit from the version in master because in master the provider ID is no longer a GUID, but I didn't want to bring that into release/2.0 as well.

Copy link

@nategraf nategraf left a comment

Choose a reason for hiding this comment

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

Additional changes need to be made to any callsite for the EventProvider destructor. To avoid more double delete issues. I recently made this change in master as part of the work in porting to Windows. (The lock changes have to do this contract issues)

The two callsites are:

  • ~EventPipeConfiguration
  • EventPipeConfiguration::DeleteDeferedProviders

4906934

@nategraf
Copy link

nategraf commented Dec 1, 2017

So I realised that the commit I link to is also incomplete. There was another issue where the use of CreateProvider was adding the config provider to the registered provider list and it would be deleted along with the other providers as well as in the destructor.

To avoid this the CreateProvider method should not be used to build the config provider. This also means some of the changes in the commit I linked above are not needed.

nategraf@28eedfe

@chunseoklee
Copy link

@brianrob @nategraf This commit will be merged or wait for enhancement ?

@nategraf
Copy link

nategraf commented Dec 29, 2017

This is intended to be merged without dependency on any other enhancements. If you are referring to the enablement of EventPipe on Windows when you say "enhancement" I don't think that change will be backported to release 2.0. (I however do not make that call)

Is this issue affecting you? If so I can take this over and get it through before Brian comes back from vacation on the 15th

@Imisnew2
Copy link

@netgraf If I may chime in, this issue is affecting me as well. It causes failures in our continuous integration infrastructure as well as for various developers. Though it can wait until the 15th or so, at least for me.

@brianrob
Copy link
Member Author

Closing this as it looks like #15765 contains this fix plus more.

@brianrob brianrob closed this Jan 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants