Skip to content

Add environment variable to override AppDomain.Unload#2192

Closed
sharwell wants to merge 1 commit intoxunit:v2from
sharwell:no-unload
Closed

Add environment variable to override AppDomain.Unload#2192
sharwell wants to merge 1 commit intoxunit:v2from
sharwell:no-unload

Conversation

@sharwell
Copy link
Copy Markdown

@sharwell sharwell commented Nov 11, 2020

This pull request provides a mitigation path for dotnet/roslyn#49024.

I considered three approaches:

  • A new option appDomainUnload, with default value true. This is the cleanest option for a user, but is difficult to implement throughout the internals without breaking API changes.
  • Changing AppDomainSupport to have an enum flag for this. This seemed like a reasonable possibility, but likely unnecessarily complex considering the current maintenance strategy for the v2 branch.
  • An environment variable. This is the simplest option, and likely only works for CI scenarios, but at least it mitigates the acute problems we're facing today.

@sharwell
Copy link
Copy Markdown
Author

@bradwilson I'm having trouble building this for test. If CI could publish a build to the package feed we can update our CI and verify over the next week that the timeouts are gone.

@bradwilson
Copy link
Copy Markdown
Member

There's no CI for v2 any more, because there are no plans to release any more v2 releases. The AppVeyor account was closed down some time ago.

@sharwell
Copy link
Copy Markdown
Author

@bradwilson I verified locally that this change fixes at least one of the common delays. This is certainly a multiple-times-per-day pain point for the Roslyn team.

@bradwilson
Copy link
Copy Markdown
Member

I could try to help figure out your build issues so that you could publish your own internal package.

@sharwell
Copy link
Copy Markdown
Author

sharwell commented Nov 12, 2020

📝 I am able to build this from dde9f82. If we were to publish a fork of the runner, it would probably just eliminate the call to Unload altogether.

@sharwell
Copy link
Copy Markdown
Author

@bradwilson this is no longer needed. We implemented a change to patch the x86/x64 assembly in memory at runtime and it met our needs.

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