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

[release/7.0] Do not use ALC name for AssemblyName in DispatchProxy #82807

Merged
merged 2 commits into from
Mar 10, 2023

Conversation

buyaa-n
Copy link
Contributor

@buyaa-n buyaa-n commented Mar 1, 2023

Backport of #82754 to release/7.0
Issue #80387

Description

DispatchProxy uses Reflection.Emit internally and used to use only one AssemblyBuilder for all proxy types. In .NET 7 that fixed with #62095, now it creates a separate AssemblyBuilder, for each AssemblyLoadContext of the proxy type and also uses the ALC name for creating AssemblyName

In some cases, ALC name could include a path and because of the special characters in the path AssemblyName constructor and throws: The given assembly name was invalid

Customer Impact

The regression causing failures for customers when they upgrade from .NET previous versions to 7.0. For now, two customers indirectly impacted by this regression:

  1. Originally reported in MSBuild repo: DispatchProxy.Create reads AssemblyLoadContext.Name but treats it as an assembly name - this has a workaround that mentioned in the issue
  2. Originally reported in WPF/Winforms repo: Executing a SOAP rating service request in a .NET 7 COM-enabled assembly fails with 'The given assembly name was invalid' - @jkotas investigated and found the same root cause, I don't think there is a workaround for this issue.

Testing

Risk

Very low, the fix is simple, in this PR we are just using the same name for each ALC, there were no specific need to pass the ALC name to the AssemblyName. Do not expect there is a code dependency on this name as this change was introduced just in 7.0.

Passing the ALC name could be useful for debugging purposes, we will fix that later appropriately as suggested this fix has a lower risk for porting into 7.0.

CC @ericstj @jeffhandley

@ghost
Copy link

ghost commented Mar 1, 2023

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #82754 to release/7.0
Issue #80387

Description

DispatchProxy uses Reflection.Emit internally and used to use only one AssemblyBuilder for all proxy types. In .NET 7 that fixed with #62095, now it creates a separate AssemblyBuilder, for each AssemblyLoadContext of the proxy type and also uses the ALC name for creating AssemblyName

In some cases, ALC name could include a path and because of the special characters in the path AssemblyName constructor and throws: The given assembly name was invalid

Customer Impact

The regression introduced with #62095 in .NET 7.0 and causing failures for some customers when they upgrade from .NET previous versions to 7.0. For now 2 customers reported this issue:

  1. Originally reported in MSBuild repo: DispatchProxy.Create reads AssemblyLoadContext.Name but treats it as an assembly name - this has workaround that mentioned in the issue
  2. Originally reported in WPF/Winforms repo: Executing a SOAP rating service request in a .NET 7 COM-enabled assembly fails with 'The given assembly name was invalid' - I don't think there is workaround for this one

Testing

  • Unit testing added with the fix
  • Manual tested with the repro in the issue

Risk

Very low, the fix is simple, this PR we are just using the same name for each ALC, there were no specific need to pass the ALC name to the AssemblyName. Do not expect there is a code dependency on this name as this change was introduced just in 7.0.

CC @ericstj @jeffhandley

Author: buyaa-n
Assignees: buyaa-n
Labels:

area-System.Reflection

Milestone: -

@rwg0
Copy link

rwg0 commented Mar 1, 2023

Hi,

I have just encountered this issue trying to port a WPF app from 6.0 to 7.0. A slightly different failure path (trying to create a DispatchProxy for a type in an assembly loaded using Assembly.LoadFrom(byte[]). Seems to be the same underlying cause of trying to parse a string as an assembly name that looks nothing like an assemblyname.

Very much in favour of this being fixed in a 7.0 update, since it's a blocking regression right now.

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Mar 2, 2023

Thanks @rwg0 that sounds similar, it would be helpful if you could verify the fix in this PR with your scenario

@rwg0
Copy link

rwg0 commented Mar 2, 2023

Thanks @rwg0 that sounds similar, it would be helpful if you could verify the fix in this PR with your scenario

I'm afraid I don't have the knowledge to build a version of the framework with the fix in to try it (or the time right now to learn how). I did go back and debug the original situation to make sure I understood it properly, and it is definitely being caused by the AssemblyLoadContext name ("Assembly.Load(byte[], ...)") tripping up the AssemblyNameParser, which expects Key=Value after the comma. I've worked around for now by creating my own AssemblyLoadContext with no name :)

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Mar 4, 2023

The failures unrelated and reported #75611 #82271

@carlossanlop
Copy link
Member

@buyaa-n the servicing branch is now open to take changes. What's the status of this PR? Does it only need a sign-off, or does it need to get more feedback addressed?

If/when it's ready, please send the email to Tactics requesting approval, if you haven't done so.

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Mar 9, 2023

The failure is unrelated and filed an issue #83226

@buyaa-n
Copy link
Contributor Author

buyaa-n commented Mar 10, 2023

@AaronRobinsonMSFT @ericstj @jkotas @steveharter could we get approval so it can get merged?

@carlossanlop
Copy link
Member

Approved by Tactics,
Signed-off by area owners.
No OOB changes needed for the involved assemblies.
CI failures all look unrelated.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit 01a25fa into release/7.0 Mar 10, 2023
@carlossanlop carlossanlop deleted the backport/pr-82754-to-release/7.0 branch March 10, 2023 16:30
@ghost ghost locked as resolved and limited conversation to collaborators Apr 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Reflection Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants