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 #4255, InternalTrace Initialize crash #4256

Merged
merged 1 commit into from Feb 21, 2023
Merged

Fix for #4255, InternalTrace Initialize crash #4256

merged 1 commit into from Feb 21, 2023

Conversation

OsirisTerje
Copy link
Member

See #4255

Copy link
Contributor

@CharliePoole CharliePoole left a comment

Choose a reason for hiding this comment

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

This seems to work for your particular use case in the adapter but changes the nature of the check previously made from ensuring that Initialization is only done once to ensuring that initialization is only done once with a level > Off.

I think the original intent (don't initialize twice) is what should be done but that the code error causing the exception should be removed. Possibly, the TraceWriter should not be initialized at all in this method, but that would require more work.

@CharliePoole
Copy link
Contributor

It seems to me that any code (whether the adapter or the engine) that calls InitializeTrace more than once is in error. While the exception should be fixed, it should not, in my view, keep us from figuring out why that call is made twice and fixing it.

@OsirisTerje
Copy link
Member Author

OsirisTerje commented Nov 17, 2022

As mentioned for the issue, the adapter is actually calling the assembly twice in its own testing. You can check this by running the red tests in the adapter, branch Fixtmppath. The tests in question load the Mockassembly and then runs the tests in that assembly, so there is a test-within-a-test here.

I can't see that the change really "changes the nature" of this. If the method is called twice, the else clause simply ignores the call, just as it did before.

@CharliePoole
Copy link
Contributor

@OsirisTerje I can see your point. The change you made is very small, I agree, but it's a small change in the semantics of the check being made. If I were re-doing this, I think I'd just throw an exception on the second initialization.

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.

None yet

3 participants