Change Assembly.GetFile(s) to throw when in-memory #40593

agocke · 2020-08-09T22:00:53Z

This code path already threw an exception in most cases, but mostly accidentally.
This changes the code to deliberately throw an exception when the assembly is
in-memory.

Fixes #40154

This is technically a breaking change, although a minor one, as the previous behavior also threw an exception. The previous exception was an IOException and this is an InvalidOperationException, so if users were catching the previous exception they would not succeed now. Fixes dotnet#40154

ghost · 2020-08-09T22:00:57Z

Tagging subscribers to this area: @vitek-karas, @agocke
See info in area-owners.md if you want to be subscribed.

src/libraries/System.Reflection/tests/AssemblyTests.cs

src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeAssembly.cs

agocke · 2020-08-10T07:45:15Z

This is a little strange -- I seem to have somehow changed the order that interface implementations here are returned. Not sure how that could have happened. Ideas?

vitek-karas · 2020-08-10T10:31:10Z

That is really weird - does it repro locally?

vitek-karas · 2020-08-10T21:22:22Z

src/mono/netcore/System.Private.CoreLib/src/System/Reflection/RuntimeAssembly.cs

@@ -425,6 +431,12 @@ public override Assembly GetSatelliteAssembly(CultureInfo culture, Version? vers

        public override FileStream[] GetFiles(bool getResourceModules)
        {
+            if (Location == "")


Nit: Maybe file an issue that this code should move to the shared corelib - for example the GetFiles is basically identical (just different coding style a bit).

I generally agree. /cc @CoffeeFlux in case I'm missing anything.

Some of this should be able to move, just have to keep in mind that changing the layout is hard because we want to match the legacy mono BCL in some cases (or at least, having them differ would result in a bunch of netcore-specific runtime code).

agocke · 2020-08-10T23:16:53Z

Huh, it still seems like interfaces have been re-ordered somehow, but I can't seem to get the mono tests to run locally

src/libraries/System.Reflection/tests/TestAssembly/TestAssembly.cs

CoffeeFlux · 2020-08-10T23:52:17Z

src/mono/netcore/System.Private.CoreLib/src/System/Reflection/RuntimeAssembly.cs

@@ -425,6 +431,12 @@ public override Assembly GetSatelliteAssembly(CultureInfo culture, Version? vers

        public override FileStream[] GetFiles(bool getResourceModules)
        {
+            if (Location == "")


Some of this should be able to move, just have to keep in mind that changing the layout is hard because we want to match the legacy mono BCL in some cases (or at least, having them differ would result in a bunch of netcore-specific runtime code).

src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeAssembly.cs

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx

Co-authored-by: Dan Moseley <danmose@microsoft.com>

…-getfiles

agocke · 2020-08-12T00:40:15Z

@danmosemsft I'm stuck on this Mono issue. Can you recommend someone to help out?

steveisok · 2020-08-12T02:27:01Z

@CoffeeFlux the mono test failures look weird. Any thoughts?

CoffeeFlux · 2020-08-12T03:25:08Z

Definitely strange. I'll take a look tomorrow.

agocke · 2020-08-13T17:22:44Z

@CoffeeFlux Any luck?

CoffeeFlux · 2020-08-14T21:24:25Z

Interestingly, these pass locally for me even with the debug configuration.

./build.sh mono+libs -c debug
./dotnet.sh build src/libraries/System.Reflection/tests/ /t:Test /p:Configuration=Debug

gives me

  ----- start Fri Aug 14 17:22:09 EDT 2020 =============== To repro directly: =====================================================
  pushd /Users/ryan/git/runtime/artifacts/bin/System.Reflection.Tests/net5.0-Debug
  /Users/ryan/git/runtime/artifacts/bin/testhost/net5.0-OSX-Debug-x64/dotnet exec --runtimeconfig System.Reflection.Tests.runtimeconfig.json  xunit.console.dll System.Reflection.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing 
  popd
  ===========================================================================================================
  ~/git/runtime/artifacts/bin/System.Reflection.Tests/net5.0-Debug ~/git/runtime/src/libraries/System.Reflection/tests
    Discovering: System.Reflection.Tests (method display = ClassAndMethod, method display options = None)
    Discovered:  System.Reflection.Tests (found 390 of 407 test cases)
    Starting:    System.Reflection.Tests (parallel test collections = on, max threads = 12)
    Finished:    System.Reflection.Tests
  === TEST EXECUTION SUMMARY ===
     System.Reflection.Tests  Total: 1278, Errors: 0, Failed: 0, Skipped: 0, Time: 1.369s
  ~/git/runtime/src/libraries/System.Reflection/tests
  ----- end Fri Aug 14 17:22:12 EDT 2020 ----- exit code 0 ----------------------------------------------------------

agocke · 2020-08-14T22:44:02Z

@CoffeeFlux The same is true for me. Should I temporarily disable this test?

CoffeeFlux · 2020-08-14T23:24:29Z

Yeah, let's disable it for now. I'm trying to get the artifacts off CI so I can figure out what's actually going on here, and maybe I'll get somewhere this weekend—think I'm done for today though. If I can't get it sorted out this weekend and it ends up being a serious issue, I'll figure out the backport.

CoffeeFlux · 2020-08-15T00:05:50Z

src/libraries/System.Reflection/tests/TypeInfoTests.cs

@@ -490,6 +490,12 @@ public void IsEnumDefined_Invalid()
        [InlineData(typeof(CompoundClass4<string>), new Type[] { typeof(GenericInterface1<string>), typeof(TI_NonGenericInterface1) })]
        public void ImplementedInterfaces(Type type, Type[] expected)
        {
+            if (PlatformDetection.IsMonoRuntime)


I believe the proper way to exclude this is to open an issue on Github with the specifics and then apply the ActiveIssue attribute to the method? That file has another example.

Sounds good

We can remove this condition since ActiveIssue is added?

I think this has been fixed as part of #40632

src/libraries/System.Reflection/tests/TypeInfoTests.cs

jkotas · 2020-08-15T04:08:12Z

Tests failing on wasm:

System.IO.FileNotFoundException : This assembly does not have a file table because it was loaded from memory.

Stack trace
   at System.Reflection.RuntimeAssembly.GetFiles(Boolean getResourceModules)
   at System.Reflection.Assembly.GetFiles()
   at System.Reflection.Tests.AssemblyTests.GetFiles()

agocke · 2020-08-15T05:41:24Z

Hmm, I'll claim that this is the right behavior -- wasm is basically load from memory. But how do I exclude these tests from running on wasm?

jkotas · 2020-08-15T05:51:04Z

You can either mark them with PlatformDetection.IsNotBrowser.

Or you can make the test robust against assemblies loaded from memory in general case and suppress the failing checks when Location is empty string. It would be nice to be able to run all tests in single file mode eventually, even outside Browser.

…-getfiles

jkotas · 2020-08-15T15:51:59Z

Opened #40885 on the test failure

agocke added the area-AssemblyLoader-coreclr label Aug 9, 2020

agocke requested a review from vitek-karas August 9, 2020 22:01

vitek-karas reviewed Aug 9, 2020

View reviewed changes

src/libraries/System.Reflection/tests/AssemblyTests.cs Outdated Show resolved Hide resolved

src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeAssembly.cs Outdated Show resolved Hide resolved

agocke added 5 commits August 9, 2020 17:32

Respond to PR comments

b1f0293

Fix typo

36edf7e

Don't specify exception for GetFile when passed null

6b195ad

Use correct xunit helper

6c315c5

Remove change to TestAssembly.cs

2760c03

Also throw in mono corlib

b5119db

agocke requested review from akoeplinger, EgorBo, marek-safar, steveisok and vargaz as code owners August 10, 2020 21:09

agocke added 2 commits August 10, 2020 14:11

Merge remote-tracking branch 'upstream/master' into throw-in-getfiles

e0f61ca

Fix whitespace

a4bc000

vitek-karas reviewed Aug 10, 2020

View reviewed changes

vitek-karas approved these changes Aug 10, 2020

View reviewed changes

CoffeeFlux reviewed Aug 10, 2020

View reviewed changes

danmoseley reviewed Aug 11, 2020

View reviewed changes

src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeAssembly.cs Outdated Show resolved Hide resolved

danmoseley reviewed Aug 11, 2020

View reviewed changes

src/libraries/System.Private.CoreLib/src/Resources/Strings.resx Outdated Show resolved Hide resolved

agocke and others added 3 commits August 10, 2020 20:47

Update src/libraries/System.Private.CoreLib/src/Resources/Strings.resx

54ca244

Co-authored-by: Dan Moseley <danmose@microsoft.com>

Respond to PR comments

5637496

Merge remote-tracking branch 'origin/throw-in-getfiles' into throw-in…

dc0f9ad

…-getfiles

Skip ImplementedInterfaces test on Mono

e5a43f4

CoffeeFlux reviewed Aug 15, 2020

View reviewed changes

agocke mentioned this pull request Aug 15, 2020

Adding code to Assembly.GetFile(s) causes interface re-ordering in Mono #40868

Closed

Add bug

0ad7e0a

CoffeeFlux approved these changes Aug 15, 2020

View reviewed changes

jkotas reviewed Aug 15, 2020

View reviewed changes

src/libraries/System.Reflection/tests/TypeInfoTests.cs Outdated Show resolved Hide resolved

Update src/libraries/System.Reflection/tests/TypeInfoTests.cs

d4278e2

agocke added 2 commits August 15, 2020 02:42

Fix tests for running in memory

90756c8

Merge remote-tracking branch 'origin/throw-in-getfiles' into throw-in…

60f591b

…-getfiles

jkotas mentioned this pull request Aug 15, 2020

JIT.Regression\JitBlue\Runtime_40444\Runtime_40444 test failure #40885

Closed

jkotas merged commit c73371d into dotnet:master Aug 15, 2020

agocke deleted the throw-in-getfiles branch August 15, 2020 18:43

karelz added this to the 5.0.0 milestone Aug 18, 2020

ghost locked as resolved and limited conversation to collaborators Dec 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Assembly.GetFile(s) to throw when in-memory #40593

Change Assembly.GetFile(s) to throw when in-memory #40593

agocke commented Aug 9, 2020 •

edited

Loading

ghost commented Aug 9, 2020

agocke commented Aug 10, 2020

vitek-karas commented Aug 10, 2020

vitek-karas Aug 10, 2020

steveisok Aug 10, 2020 •

edited

Loading

CoffeeFlux Aug 10, 2020

agocke commented Aug 10, 2020

CoffeeFlux Aug 10, 2020

agocke commented Aug 12, 2020

steveisok commented Aug 12, 2020

CoffeeFlux commented Aug 12, 2020

agocke commented Aug 13, 2020

CoffeeFlux commented Aug 14, 2020

agocke commented Aug 14, 2020

CoffeeFlux commented Aug 14, 2020

CoffeeFlux Aug 15, 2020

agocke Aug 15, 2020

am11 Aug 15, 2020

wzchua Aug 15, 2020 •

edited

Loading

jkotas commented Aug 15, 2020

agocke commented Aug 15, 2020

jkotas commented Aug 15, 2020

jkotas commented Aug 15, 2020

Change Assembly.GetFile(s) to throw when in-memory #40593

Change Assembly.GetFile(s) to throw when in-memory #40593

Conversation

agocke commented Aug 9, 2020 • edited Loading

ghost commented Aug 9, 2020

agocke commented Aug 10, 2020

vitek-karas commented Aug 10, 2020

vitek-karas Aug 10, 2020

Choose a reason for hiding this comment

steveisok Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

CoffeeFlux Aug 10, 2020

Choose a reason for hiding this comment

agocke commented Aug 10, 2020

CoffeeFlux Aug 10, 2020

Choose a reason for hiding this comment

agocke commented Aug 12, 2020

steveisok commented Aug 12, 2020

CoffeeFlux commented Aug 12, 2020

agocke commented Aug 13, 2020

CoffeeFlux commented Aug 14, 2020

agocke commented Aug 14, 2020

CoffeeFlux commented Aug 14, 2020

CoffeeFlux Aug 15, 2020

Choose a reason for hiding this comment

agocke Aug 15, 2020

Choose a reason for hiding this comment

am11 Aug 15, 2020

Choose a reason for hiding this comment

wzchua Aug 15, 2020 • edited Loading

Choose a reason for hiding this comment

jkotas commented Aug 15, 2020

agocke commented Aug 15, 2020

jkotas commented Aug 15, 2020

jkotas commented Aug 15, 2020

agocke commented Aug 9, 2020 •

edited

Loading

steveisok Aug 10, 2020 •

edited

Loading

wzchua Aug 15, 2020 •

edited

Loading