-
Notifications
You must be signed in to change notification settings - Fork 62
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
Work around 64 bit RyuJIT ThreadAbortException bug on the .NET Framework #137
Conversation
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... |
@jdasilva-olo: another commit is fine, thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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. 🤷♂️
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
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.