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

Work around 64 bit RyuJIT ThreadAbortException bug on the .NET Framework #137

Merged

Conversation

jdasilva-olo
Copy link
Contributor

This fixes #136

I had to add net461 as a target framework to the tests to be able to get a failing test. A few additional minor changes to the test code were needed to make it compile.

The new test only has the potential to fail in 64-bit release mode using RyuJIT. With ReleaseMode selected Resharper uses 64-bit by default, but for Visual Studio you can select 64-bit in the Process Architecture for AnyCPU projects menu option.

ThreadAbortException exists but is not used in .NET Core so this change will have no impact.

If you comment out the fix and since it's release mode add a Console.WriteLine here, you should be able to see the constant output of repeating ThreadAbortExceptions.

@jdasilva-olo
Copy link
Contributor Author

I have a fix for the style issue in the CI build ready. Let me know if you'd prefer a force push or another commit...

@lucaspimentel
Copy link
Member

@jdasilva-olo: another commit is fine, thanks.

Copy link
Contributor

@ogaca-dd ogaca-dd left a comment

Choose a reason for hiding this comment

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

@jdasilva-olo ,

Thanks a lot for this PR!
It looks good to me, just having a single question.

@@ -111,6 +112,15 @@ private void Dequeue()
}
}
}
#if !NETSTANDARD1_3
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you give more information on why using !NETSTANDARD1_3? Should it be NETFRAMEWORK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good catch, I don't think there was a reason. NETFRAMEWORK will be better for adding/changing targets, which I did use on the test. 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, so I remembered today that I was thinking since ThreadAbortException exists in .NET Standard 2.0, and a .NET Standard DLL could run under the .NET Framework this would allow the fix to work in that scenario, while the extra code would be meaningless when it ran under .NET Core. That said, since you include assemblies targeting the .NET Framework in the NuGet, I'm not sure you can get to that scenario under normal circumstances.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the detailed explanation!

@ogaca-dd ogaca-dd merged commit 822d895 into DataDog:master Nov 13, 2020
@jdasilva-olo jdasilva-olo deleted the jdasilva-olo/thread-abort-workaround branch November 13, 2020 12:02
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.

When DogStatsdService is not disposed in an AppDomain, the AppDomain cannot be unloaded
3 participants