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: If GetCurrentAppDomainId fails try GetDefaultDomain first (.netframework remote start issue) #178

Conversation

mitchcapper
Copy link
Contributor

@mitchcapper mitchcapper commented Aug 31, 2023

Thanks for a fantastic library!
This does 3 things, if you want it split into 3 commits let me know:

  • Adds the cpp platform to the solution/project as it is what is generally used anyway
  • Checks the result of GetCurrentAppDomain() call, for whatever reason the result was ignored before, maybe to default to 0 incase of failure, but this can lead to startup aborts later when the appdomain is incorrect (adds a few more error states as well)
  • Fix an issue with loading in a .net framework application via a new remote thread. I found GetCurrentAppDomain would fail and couldn't find a good way to fix this. The other calls all succeeded. I did find that if I called GetDefaultDomain on the old runtime interface this would fix it so the call would then succeed. Technically if we import the AppDomain type we could pull the id right off of the result from GetDefaultDomain but not sure its needed. This should only change the hotpath for previous where the code would gloriously fail before (well assuming the appdomain was not 0, but the more commonly 1 for .net framework apps anyway).

Copy link
Owner

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

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

This looks great, thank you! I didn't test the remote thread activation path. There is also a lot of confusion around this style of activation in in .NET Framework. If the below works for you, please commit the suggestions and I will merge the PR.

src/platform/dnne.h Outdated Show resolved Hide resolved
src/platform/platform_v4.cpp Show resolved Hide resolved
src/platform/platform_v4.cpp Outdated Show resolved Hide resolved
@mitchcapper
Copy link
Contributor Author

So sorry forgot to do as requested and follow up on this!

@AaronRobinsonMSFT
Copy link
Owner

So sorry forgot to do as requested and follow up on this!

No worries. We all get busy with the constant in/out of PRs and requested changes. I didn't want to step on your effort so I didn't do it myself. However, I like this fix and want to get it in this week :-)

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit fdff3d4 into AaronRobinsonMSFT:master Nov 6, 2023
6 checks passed
@AaronRobinsonMSFT
Copy link
Owner

Thanks @mitchcapper !

@AaronRobinsonMSFT
Copy link
Owner

@mitchcapper A new package with these changes has been release - https://www.nuget.org/packages/DNNE/2.0.6

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