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: Don't fail metadata updates on missing assemblies #40725

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jeromelaban
Copy link
Contributor

This change is about https://developercommunity.visualstudio.com/t/WSL-HotReload-does-not-invoke-MetadataUp/10643733, where metadata updates may fail when assemblies cannot be found in a cross-platform scenario.

In failure context, an app is built from Visual Studio, then executed on Linux, where WPF assemblies are not present, but some non-loaded assemblies reference WPF. When metadata update gets applied, searching for the MetadataUpdateHandler may fail because some dependencies may not be found when Assembly.GetCustomAttributesData() is called and fails to find the rest of the handlers which stops invoking MetadataUpdateHandler types.

Skipping the assembly entirely allows for hot reload to successfully completely.

This change is technically not useful for dotnet watch, but I understand from @danroth27 that these files are synchronized to Visual Studio, where the above scenario applies.

Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Request triage from a team member label May 7, 2024
@ViktorHofer
Copy link
Member

ViktorHofer commented May 8, 2024

The sdk-unified-build test failure is unrelated and will go away when you update your branch to target latest main.

@jeromelaban
Copy link
Contributor Author

Looks like it did the trick, thanks @ViktorHofer!

@phil-allen-msft phil-allen-msft requested a review from tmat May 31, 2024 22:47
// In such case, we can ignore the assemblies and continue enumerating handlers for
// the rest of the assemblies of current domain.
_log($"'{assembly.FullName}' is not loaded ({e.Message})");
continue;
Copy link
Member

Choose a reason for hiding this comment

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

nit: continue is not needed.

@tmat
Copy link
Member

tmat commented Jun 28, 2024

It'd be good to add a test. The test can compile a project that has a dependent project and delete the dependent dll from the output directory before making the source change. That should emulate the scenario well. ApplyDeltaTests.HandleTypeLoadFailure is already testing a similar scenario. Perhaps you could parameterize the test to target this scenario as well.

@jeromelaban
Copy link
Contributor Author

ApplyDeltaTests.HandleTypeLoadFailure is already testing a similar scenario

@tmat I've been trying to enable the test for this specific scenario, but HandleTypeLoadFailure has been disabled recently because of #42850 and I can't run this new test for the same reason. I'll make a change that does the same, but do you have a known way to test this specific scenario regardless, or do we need to wait for net10?

@jeromelaban jeromelaban marked this pull request as ready for review September 10, 2024 15:37
@tmat
Copy link
Member

tmat commented Sep 10, 2024

@jeromelaban Let me try to reenable the tests. I was on vacation when they got disabled.

@tmat
Copy link
Member

tmat commented Sep 12, 2024

@jeromelaban Turns out main is currently transitioning to .NET 10 and has some version inconsistencies between runtime and the SDK, which causes issues with some dotnet-watch tests. See #42920 for status.

Would you mind rebasing your changes to release/9.0.1xx branch? Thanks for patience!

@jeromelaban
Copy link
Contributor Author

@tmat Sure, thanks for the investigation! Do you want me to retarget this PR, or to make another one targeting 9.0.1xx and keep this one on main as well?

@tmat
Copy link
Member

tmat commented Sep 12, 2024

Either way works.

@jeromelaban
Copy link
Contributor Author

@tmat Now that the other PR is merged, does this PR still make sense? I'm not sure how backports work there. Thanks!

@jeromelaban
Copy link
Contributor Author

@tmat I see that the backport that was done to 9.0.x did not get merged to main. My understanding is that I should update this PR so that it stays in 10..

Also, it seems that this change did not end up being included in VS 17.12, would you know if it will be in 17.13?

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants