[mono] Add test for wasm loader regression, fix loading from bundle #42425
Conversation
As-is, this will fail on Browser
This test relies on loading corelib from disk via Assembly.Location, none of which works on Browser. It passing historically is a quirk of our previously broken implementation.
Load after checking the default ALC, non-satellite loads go into the default ALC, satellite loads go into the parent assembly's ALC (following the managed logic)
CoffeeFlux
requested review from
lambdageek,
thaystg and
vargaz
as code owners
|
Tagging subscribers to this area: @CoffeeFlux |
|
@CoffeeFlux what other changes would we have to backport? |
|
Option 1: Get things completely correct. We'd need to backport this, #41358, and a small PR I'm about to submit to aspnetcore (and by that I mean after I sleep). Option 2: Manually backport this PR on top of RC2, fixing the ALC selection issues and ignoring the satellite assembly culture problems addressed by the previous PR. I lean towards this since it seems to minimize risk while fixing a clear issue with custom ALC usage, but it's significantly more work for me. Option 3: Backport this PR and #41358. This would produce a much larger diff than option 2, and with it significantly higher risk, but fixes a couple extra issues related to satellite assemblies and is also essentially zero extra work. After giving it some thought, I lean towards option 2 here since I still don't think the satellite assembly issues are pressing enough to merit the associated risk and am less concerned about the risk associated with a manual backport than I was a few hours ago; the diff shouldn't be that large outside of the new test. Additionally, we have a direct customer bug report for this, whereas the satellite assembly bugs were just found via failing tests. All that said, I'm heading to bed so I guess I'll see how I feel after getting some rest. If you also think option 2 is the way to go here, I'll start on that when I get up. |
|
@CoffeeFlux I don't understand #41358. (see #41358 (comment)). It seems like we are registering multiple assemblies in the bundle with the same path name and then expecting all the "by path" low-level loading routines to be culture aware, and then running into problems because we've conflated "by assembly name" and "by file path" loading. Assuming I understand the problem (not a given), the correct fix is to change the bundling process to use distinct path names for distinct assemblies. (For assembly Alternately, we should just bypass all the "by path" loading when loading from bundles. I'm not sure how to do that, however. |
|
After sleep and some discussion with Aleksey, I think we should definitely not backport anything other than this. I'll do it manually once we reach a consensus and land this individual PR. |
There was a problem hiding this comment.
Had an offline discussion with @CoffeeFlux and this PR makes sense to me.
For the backport I like Option 2 (do a backport by hand that doesn't include the bundled satellite assembly changes from #41358). The satellite assemblies change seems riskier and I don't entirely follow the consequences to MonoLoadedImages caches from the changes there.
| reference = search_bundle_for_assembly (mono_domain_default_alc (mono_alc_domain (alc)), aname); | ||
| if (reference) { | ||
| mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_ASSEMBLY, "Assembly found in the bundle: '%s'.", aname->name); | ||
| goto leave; | ||
| } | ||
| } |
There was a problem hiding this comment.
ok this part makes sense to me.
|
/azp run runtime |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ghost
locked as resolved and limited conversation to collaborators
Fixes #42207
The new test is based on the sample from that issue.
The naive fix (moving all bundle loading to after we check the default ALC) highlights that the satellite assembly tests were passing for the wrong reasons. The full fix here handles satellite loads correctly, putting them in the same ALC as the parent assembly. One non-satellite test that was previously passing also passed erroneously, but it makes no sense on wasm so disable it.
I don't know how likely we think customers are to use custom ALCs on wasm, but the current behavior seems fairly broken. For the repro I use
Assembly.Load(byte[])to match the customer issue, but I think you can achieve the same effect by just loading an assembly into a custom ALC and trying to cast the same way.If we do want to backport, the problem is these changes are all on top of @safern's previous bundle work, so I'd either need to backport the whole thing or manually implement some of this work on top of RC2. Maybe the latter option is the way to go here? Not sure how to proceed. cc: @marek-safar @SamMonoRT