Skip to content

Comments

TraceEvent - don't use undocumented APIs#1802

Merged
brianrob merged 4 commits intomicrosoft:mainfrom
idigdoug:useDocumentedApis
Feb 9, 2023
Merged

TraceEvent - don't use undocumented APIs#1802
brianrob merged 4 commits intomicrosoft:mainfrom
idigdoug:useDocumentedApis

Conversation

@idigdoug
Copy link
Contributor

@idigdoug idigdoug commented Feb 7, 2023

Stop using ETWControl and ETWParsing from OSExtensions.dll. Replace them with implementations that use Microsoft-supported APIs.

  • Use TraceSetInformation for configuring profile interval (no change from existing behavior other than using supported API).
  • Use TraceSetInformation for configuring stack caching (same behavior, except that the new API is only supported on Windows 10 19582+).
  • Use TdhEnumerateManifestProviderEvents for reading manifest data (works perfectly for manifests, but doesn't work for MOF).

Still using OSExtensions.dll for KernelTraceControl stuff, but that's supported by Microsoft. So perhaps the KernelTraceControl stuff could move into TraceEvent and then OSExtensions.dll would no longer be needed at all.

Doug Cook (WINDOWS) added 2 commits February 4, 2023 14:30
Stop using ETWControl and ETWParsing from OSExtensions.dll. Replace them
with implementations that use Microsoft-supported APIs.

- Use TraceSetInformation for configuring profile interval (no change
  from existing behavior other than using supported API).
- Use TraceSetInformation for configuring stack caching (same behavior,
  except that the new API is only supported on Windows 10 19582+).
- Use TdhEnumerateManifestProviderEvents for reading manifest data
  (works perfectly for manifests, but doesn't work for MOF).

Still using OSExtensions.dll for KernelTraceControl stuff, but that's
supported by Microsoft. So perhaps the KernelTraceControl stuff could
move into TraceEvent and then OSExtensions.dll would no longer be needed
at all.
@pharring
Copy link
Member

pharring commented Feb 7, 2023

@brianrob code LGTM, but over to you to decide if this is moving the right direction vis-a-vis OSExtensions.dll (and possible support for downlevel OSes)

@brianrob
Copy link
Member

brianrob commented Feb 7, 2023

Thanks for reviewing @pharring. I am in the process of testing this to see how it impacts existing scenarios.

@pharring
Copy link
Member

pharring commented Feb 8, 2023

The PerfCollect unit tests are failing due to a leaking SafeTraceHandle. Implying that we forgot to Dispose a SafeTraceHandle.

@idigdoug
Copy link
Contributor Author

idigdoug commented Feb 8, 2023

This is puzzling to me because my change does not create or dispose any handles.

@pharring
Copy link
Member

pharring commented Feb 8, 2023

Are you able to run unit tests locally in VS?

I'm taking more than a passing interest in this because I introduced the SafeTraceHandle quite recently. There could certainly be a bug there or there could be legitimate cases where leaking a SafeTraceHandle is necessary or unavoidable.
If you click through the "Details" of the failing build and then through to the build log (assuming you have access), you can get more details, but personally, I can't determine exactly which test failed. The leak is reported as a debug assert, but it looks like that doesn't result in a specific test failure. Of course, finalizers can run any time, and most likely (long) after the culpable test has completed. I think XUnit calls GC.Collect and WaitForPendingFinalizers, but I'm not certain it's after every test case. All you can know is that it's at least one of the tests prior to the Debug.Assert.

@pharring
Copy link
Member

pharring commented Feb 8, 2023

Good job fixing the leak, @idigdoug Thank you!

Copy link
Member

@brianrob brianrob left a comment

Choose a reason for hiding this comment

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

Thanks @idigdoug! I did some testing with this and all looks good.

@brianrob brianrob added this pull request to the merge queue Feb 9, 2023
Merged via the queue into microsoft:main with commit 247a8a2 Feb 9, 2023
@idigdoug idigdoug deleted the useDocumentedApis branch February 10, 2023 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants