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.

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

Fix for issue #1767 #1869

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

ryandle
Copy link
Member

@ryandle ryandle commented May 10, 2023

This change updates GetActiveSessionNames to check for ERROR_MORE_DATA and retry once when calling QueryAllTraces.

I chose to implement this by modifying the current function as little as possible. To do that, I wrapped the core logic and made a recursive call to it for the retry, with a depth counter that maxes out at 1.

This change also has a unit test, but I don't think it is ready to be committed. I am including it so a discussion can be had about it.

In general it seems hard to reliably simulate this condition on a broad set of machines that will run this test suite (65+ active etw sessions while EtwMaxLoggers registry key is not set.) Just between my two developer machines I couldn't do it 🤣

I'm open to ideas here on how the test can be improved to be more reliable, otherwise it probably needs to be removed.

This change updates GetActiveSessionNames to check for ERROR_MORE_DATA and retry once when calling QueryAllTraces.

I chose to implement this by modifying the current function as little as possible. To do that, I wrapped the core logic and made a recursive call to it for the retry, with a depth counter that maxes out at 1.

This change also has a unit test, but I don't think it is ready to be comitted. I am including it so a discussion can be had. In general it seems hard to reliably simulate this condition on a broad set of machines that will run this test suite (65+ active etw sessions while EtwMaxLoggers registry key is not set.)
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.

@ryandle, thank you for contributing this, and sorry for the delay in reviewing it. Overall, the change to TraceEventSession looks good to me. I did make a change in this space that is conflicting with your change - can you please update your code to consume the change that allocates propertiesArray on the heap?

As far as the unit test goes, I think you are going to have trouble getting this to work almost everywhere. The only real option I see is to mock it. I would say that at this point, you can just remove the unit test, but it is important to make sure that this actually works in practice. Have you seen this work on machine that does have this many sessions?

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.

2 participants