[Java.Interop.Tools.Cecil] Fix DirectoryAssemblyResolver.Dispose() #88

jonpryor · 2016-09-23T16:51:51Z

Context: #87 (comment)
Context: https://bugzilla.xamarin.com/show_bug.cgi?id=44529

(Private) Bug #44529 is an IOException on xamarin-android/master
due to file sharing:

Error executing task StripEmbeddedLibraries: System.IO.IOException: Sharing violation on path .../obj/Release/linksrc/Xamarin.Android.NUnitLite.dll
  at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share, System.Int32 bufferSize, System.Boolean anonymous, System.IO.FileOptions options) [0x0025f] in <253a3790b2c44512bbca8669ecfc1822>:0
  at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share) [0x00000] in <253a3790b2c44512bbca8669ecfc1822>:0
  at (wrapper remoting-invoke-with-check) System.IO.FileStream:.ctor (string,System.IO.FileMode,System.IO.FileAccess,System.IO.FileShare)
  at Mono.Cecil.ModuleDefinition.GetFileStream (System.String fileName, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share) [0x00007] in :0
  at Mono.Cecil.ModuleDefinition.Write (System.String fileName, Mono.Cecil.WriterParameters parameters) [0x00007] in :0
  at Mono.Cecil.AssemblyDefinition.Write (System.String fileName, Mono.Cecil.WriterParameters parameters) [0x00001] in :0
  at Xamarin.Android.Tasks.StripEmbeddedLibraries.Execute () [0x0034a] in <3d5202a5d4874a76a99388021bf1ab1a>:0

The underlying cause of this change is the migration to
Cecil 0.10.0-beta1-v2 (dfed286), which -- along with API changes --
has some semantic changes ¹.

In particular, within Cecil <= 0.9.x, AssemblyDefinition was
entirely in-memory. Starting with Cecil 0.10.x, AssemblyDefinition
isn't; it is backed by a System.IO.Stream, which can be in-memory
(if ReaderParameters.InMemory is true), or a FileStream
(the default).

This normally might not be bad, except we also have
Java.Interop.Tools.Cecil.DirectoryAssemblyResolver, which caches
all created AssemblyDefinition instances.

Thus, "normal" correct Cecil use would be fine, so long as you know
all assemblies which have been loaded, load them with the correct
ReaderParameters, and promptly Dispose() of the
AssemblyDefinition when done.

DirectoryAssemblyResolver throws a wrench in that, because
(1) commit dfed286 incorrectly implemented
DirectoryAssemblyResolver.Dispose(), leaving all of the cached
AssemblyDefinition instances still "live", which means
(2) The lifetime of the Stream underlying the AssemblyDefinition
is controlled by the GC, which can mean nearly anything.

This is all a huge recipe for confusion.

Fix DirectoryAssemblyResolver.Dispose() so that the cached
AssemblyDefinition instances are Dispose()d of, and review all use
of DirectoryAssemblyResolver within Java.Interop to ensure that any
created instances are appropriate Dispose()d of.

Additionally, add a new DirectoryAssemblyResolver constructor to
allow controlling the ReaderParameters that
DirectoryAssemblyResolver.Load() uses when loading an assembly:

partial class DirectoryAssemblyResolver {
    public DirectoryAssemblyResolver (
        Action<string, object[]>  logWarnings,
        bool                      loadDebugSymbols,
        ReaderParameters          loadReaderParameters = null);
}

The new loadReaderParameters allows specifying the default
ReaderParameters values to use when
DirectoryAssemblyResolver.Load() is invoked. This ensures that all
assemblies loaded by DirectoryAssemblyResolver are loaded in a
consistent fashion (e.g. readonly, read+write, in-memory), which will
hopefully allow code to be sanely reasoned about.

The best kind of changes! ↩

marek-safar · 2016-09-23T16:56:43Z

src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs

@@ -114,9 +127,22 @@ public virtual AssemblyDefinition Load (string fileName)

 		protected virtual AssemblyDefinition ReadAssembly (string file)
 		{
+			bool haveDebugSymbols = loadDebugSymbols &&
+				(File.Exists (Path.ChangeExtension (file, ".mdb")) ||


This is wrong. mdb is not extension change but + ".mdb"

marek-safar · 2016-09-23T16:57:27Z

src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs

+			bool haveDebugSymbols = loadDebugSymbols &&
+				(File.Exists (Path.ChangeExtension (file, ".mdb")) ||
+				 File.Exists (Path.ChangeExtension (file, ".pdb")) ||
+				 File.Exists (Path.ChangeExtension (file, ".ppdb")));


.ppdb is what ?

Isn't .ppdb the file extension for Portable PDB files?

no, there is only .pdb

marek-safar · 2016-09-23T16:58:22Z

src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs

+				InMemory                        = loadReaderParameters.InMemory,
+				MetadataResolver                = loadReaderParameters.MetadataResolver,
+				ReadingMode                     = loadReaderParameters.ReadingMode,
+				ReadSymbols                     = haveDebugSymbols,


Would be better to set to true and leave it to Cecil to decide which one to load

As far as I can quickly tell, that won't work; it looks like Cecil will throw an exception if ReadSymbols is true and the .mdb file doesn't exist.

To verify:

$ csharp -r:Mono.Cecil.dll Mono C# Shell, type "help;" for help Enter statements below. csharp> using Mono.Cecil; csharp> var rp = new ReaderParameters { ReadSymbols = true }; csharp> var ad = AssemblyDefinition.ReadAssembly ("Mono.Cecil", rp); System.IO.FileNotFoundException: Could not find file "/Users/jon/Dropbox/Developer/Java.Interop/bin/Debug/Mono.Cecil". File name: '/Users/jon/Dropbox/Developer/Java.Interop/bin/Debug/Mono.Cecil' at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share, System.Int32 bufferSize, System.Boolean anonymous, System.IO.FileOptions options) [0x0021a] in <94fd79a3b7144c54b4cb162b50fc7761>:0 at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share) [0x00000] in <94fd79a3b7144c54b4cb162b50fc7761>:0 at (wrapper remoting-invoke-with-check) System.IO.FileStream:.ctor (string,System.IO.FileMode,System.IO.FileAccess,System.IO.FileShare) at Mono.Cecil.ModuleDefinition.GetFileStream (System.String fileName, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share) [0x00006] in <c7a3ce423ee8414bbda752a60dd297fd>:0 at Mono.Cecil.ModuleDefinition.ReadModule (System.String fileName, Mono.Cecil.ReaderParameters parameters) [0x00008] in <c7a3ce423ee8414bbda752a60dd297fd>:0 at Mono.Cecil.AssemblyDefinition.ReadAssembly (System.String fileName, Mono.Cecil.ReaderParameters parameters) [0x00000] in <c7a3ce423ee8414bbda752a60dd297fd>:0 at <InteractiveExpressionClass>.Host (System.Object& $retval) [0x00000] in <1596036310d3473886b65733f6439422>:0 at Mono.CSharp.Evaluator.Evaluate (System.String input, System.Object& result, System.Boolean& result_set) [0x0003e] in <984452ea6c444474a99da68d7572368e>:0 at Mono.CSharpShell.Evaluate (System.String input) [0x00000] in <693eac6b8b7545318543ea23138aba75>:0

I suppose we could always set it to true anyway, given that we catch the exception, set reader_parameters.ReadSymbols = false, and try again, but that seems wasteful.

We cannot produce pdb on mono without this being fixed. There is PR waiting for approval so any any cecil based tool works properly on mono.

See jbevain/cecil#293

I'm sorry, but I don't understand the comment; what does the "this" refer to in "without this being fixed"?

My guess is that "this" refers to "ReaderParameters.ReadSymbols always set to true".

Assuming that's the case, I'm still somewhat dubious of that. The current expectation -- if annoying -- is that when ReaderParameters.ReadSymbols is true and the symbol file doesn't exist, a FileNotFoundException is thrown. Changing that semantic would likely annoy anybody who actually wants to ensure that symbol files exist.

There are thus two solutions I can think of:

Always set ReaderParameters.ReadSymbols to true anyway, then catch the exception. (Just because I don't like it doesn't mean it's not the better solution.)

Add a new ReaderParameters.ReadSymbolsWhenPossible property (or something), which would cause Cecil to try to read symbols if they exist, but not fail if they don't.

By "this" I meant forced mdb debug symbols reader on Mono. Cecil is missing implementation of pdb/mdb reading logic and I don't know yet which option will jb prefer but ReadSymbolsWhenPossible will probably be the winner. Until this is implemented you can check for mdb file presence and set optionally based on that (although it's still wrong as there could be mdb vs dll mismatch it should cover most cases).

This can be revisited once cecil got relevant implementation.

lewurm · 2016-09-23T17:01:37Z

tools/generator/CodeGenerator.cs

@@ -278,8 +278,10 @@ public static void Run (CodeGeneratorOptions options)
 				apiXmlFile = api_xml_adjuster_output ?? Path.Combine (Path.GetDirectoryName (filename), Path.GetFileName (filename) + ".adjusted");
 				new Adjuster ().Process (filename, SymbolTable.AllRegisteredSymbols ().OfType<GenBase> ().ToArray (), apiXmlFile);
 			}
-			if (only_xml_adjuster)
+			if (only_xml_adjuster) {
+				resolver.Dispose ();


any reason why you aren't putting resolver in a using statement?

It would re-indent the entire method. :-)

I thought of a different way to ensure disposal w/o re-indenting hundreds of lines...

Fixed.

lewurm · 2016-09-23T17:03:51Z

LGTM. thanks for taking this thing over 👍

Context: dotnet#87 (comment) Context: https://bugzilla.xamarin.com/show_bug.cgi?id=44529 (Private) Bug #44529 is an `IOException` on `xamarin-android/master` due to file sharing: Error executing task StripEmbeddedLibraries: System.IO.IOException: Sharing violation on path .../obj/Release/linksrc/Xamarin.Android.NUnitLite.dll at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share, System.Int32 bufferSize, System.Boolean anonymous, System.IO.FileOptions options) [0x0025f] in <253a3790b2c44512bbca8669ecfc1822>:0 at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share) [0x00000] in <253a3790b2c44512bbca8669ecfc1822>:0 at (wrapper remoting-invoke-with-check) System.IO.FileStream:.ctor (string,System.IO.FileMode,System.IO.FileAccess,System.IO.FileShare) at Mono.Cecil.ModuleDefinition.GetFileStream (System.String fileName, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share) [0x00007] in :0 at Mono.Cecil.ModuleDefinition.Write (System.String fileName, Mono.Cecil.WriterParameters parameters) [0x00007] in :0 at Mono.Cecil.AssemblyDefinition.Write (System.String fileName, Mono.Cecil.WriterParameters parameters) [0x00001] in :0 at Xamarin.Android.Tasks.StripEmbeddedLibraries.Execute () [0x0034a] in <3d5202a5d4874a76a99388021bf1ab1a>:0 The underlying cause of this change is the migration to Cecil 0.10.0-beta1-v2 (dfed286), which -- along with API changes -- has some *semantic* changes [^0]. In particular, within Cecil <= 0.9.x, `AssemblyDefinition` was entirely in-memory. Starting with Cecil 0.10.x, `AssemblyDefinition` isn't; it is backed by a `System.IO.Stream`, which can be in-memory (if `ReaderParameters.InMemory` is `true`), or a `FileStream` (the default). This normally might not be bad, except we also have `Java.Interop.Tools.Cecil.DirectoryAssemblyResolver`, which *caches* all created `AssemblyDefinition` instances. Thus, "normal" *correct* Cecil use would be fine, so long as you know all assemblies which have been loaded, load them with the correct `ReaderParameters`, and promptly `Dispose()` of the `AssemblyDefinition` when done. `DirectoryAssemblyResolver` throws a wrench in that, because (1) commit dfed286 incorrectly implemented `DirectoryAssemblyResolver.Dispose()`, leaving all of the cached `AssemblyDefinition` instances still "live", which means (2) The lifetime of the `Stream` underlying the `AssemblyDefinition` is controlled by the GC, which can mean nearly anything. This is all a *huge* recipe for confusion. Fix `DirectoryAssemblyResolver.Dispose()` so that the cached `AssemblyDefinition` instances are `Dispose()`d of, and review all use of `DirectoryAssemblyResolver` within Java.Interop to ensure that any created instances are appropriate `Dispose()`d of. Additionally, add a new `DirectoryAssemblyResolver` constructor to allow controlling the `ReaderParameters` that `DirectoryAssemblyResolver.Load()` uses when loading an assembly: partial class DirectoryAssemblyResolver { public DirectoryAssemblyResolver ( Action<string, object[]> logWarnings, bool loadDebugSymbols, ReaderParameters loadReaderParameters = null); } The new `loadReaderParameters` allows specifying the default `ReaderParameters` values to use when `DirectoryAssemblyResolver.Load()` is invoked. This ensures that all assemblies loaded by `DirectoryAssemblyResolver` are loaded in a consistent fashion (e.g. readonly, read+write, in-memory), which will hopefully allow code to be sanely reasoned about. [^0]: The best kind of changes!

…nces Context: https://bugzilla.xamarin.com/show_bug.cgi?id=44529 Commit 1d65825 bumped to Cecil 0.10.0-beta1-v2 and Java.Interop/a1d3ecc8, which likewise uses Cecil 0.10.0-beta1-v2. Among the various API changes in Cecil 0.10.x vs. Cecil 0.9.x is a change to `Mono.Cecil.IAssemblyResolver`, implemented by `Java.Interop.Tools.Cecil.DirectoryAssemblyResolver`, to implement the `IDisposable` interface. Java.Interop/a1d3ecc8 added a `DirectoryAssemblyResolver.Dispose()` method, but this method didn't *do* anything. Cecil 0.10.0-beta1-v2 also has a *semantic* change vs. Cecil 0.9.x: `AssemblyDefinition` is no longer a "purely in-memory" construct. Instead, it holds a `System.IO.Stream` to the assembly it's reading (unless `Mono.Cecil.ReaderParameters.InMemory` is `true`). This semantic change means that an assembly can't be read from and written to at the same time via different mechanisms, so long as there is an `AssemblyDefinition` instance to that file. Which brings us to (private) Bug #44529, in which an `IOException` is thrown from the `<StripEmbeddedLibraries/>` task: Error executing task StripEmbeddedLibraries: System.IO.IOException: Sharing violation on path .../obj/Release/linksrc/Xamarin.Android.NUnitLite.dll at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share, System.Int32 bufferSize, System.Boolean anonymous, System.IO.FileOptions options) [0x0025f] in <253a3790b2c44512bbca8669ecfc1822>:0 at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share) [0x00000] in <253a3790b2c44512bbca8669ecfc1822>:0 at (wrapper remoting-invoke-with-check) System.IO.FileStream:.ctor (string,System.IO.FileMode,System.IO.FileAccess,System.IO.FileShare) at Mono.Cecil.ModuleDefinition.GetFileStream (System.String fileName, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share) [0x00007] in :0 at Mono.Cecil.ModuleDefinition.Write (System.String fileName, Mono.Cecil.WriterParameters parameters) [0x00007] in :0 at Mono.Cecil.AssemblyDefinition.Write (System.String fileName, Mono.Cecil.WriterParameters parameters) [0x00001] in :0 at Xamarin.Android.Tasks.StripEmbeddedLibraries.Execute () [0x0034a] in <3d5202a5d4874a76a99388021bf1ab1a>:0 The underlying cause of the `IOException` is twofold: 1. `AssemblyDefinition` instances now hold a `FileStream` to the file they're reading from (and optionally writing to), and this `FileStream` will be kept open until `AssemblyDefinition.Dispose()` is called. 2. Nothing in `Xamarin.Android.Build.Tasks` calls `AssemblyDefinition.Dispose()`. Nor should anything in `Xamarin.Android.Build.Tasks` call `AssemblyDefinition.Dispose()`! (Almost) All `AssemblyDefinition` instances are *cached* within `DirectoryAssemblyResolver` instances, and nothing calls `DirectoryAssemblyResolver.Dispose()` either! (Not that this matters *too* much, becuase `DirectoryAssemblyResolver.Dispose()` is currently a no-op.) There is no sane reasoning here: so long as we're using `DirectoryAssemblyResolver`, we're going to have more files open (compared to when Cecil 0.9.x was used), which can result in file sharing problems and the above `IOException`. The way foward is: 1. `Java.Interop.Tools.Cecil.DirectoryAssemblyResolver` needs to be updated so that [`DirectoryAssemblyResolver.Dispose()` is useful][0], i.e. that it will `Dispose()` of all held `AssemblyDefinition` instances. 2. The Java.Interop subproject reference needs to be updated to pick up (1). TODO: do this. ;-) 3. `Xamarin.Android.Build.Tasks.dll` needs to be updated so that `DirectoryAssemblyResolver.Dispose()` is used. 4. All code, in particular `Xamarin.Andorid.Build.Tasks.dll`, needs to be reviewed to ensure it doesn't do anything "stupid" with `AssemblyDefinition` instances and/or `DirectoryAssemblyResolver.` Examples of "stupid" things include: * `Dispose()`ing of the return value from `DirectoryAssemblyResolver.Load()`: resolver.Load (assemblyName).Dispose(); * Treating `AssemblyDefinition` instances in a manner inconsistent with how `DirectoryAssemblyResolver` loads them. `DirectoryAssemblyResolver` will be getting a new constructor overload which takes an `Mono.Cecil.ReaderParameters` parameter; the default will be the default Cecil behavior, which is for read-only behavior. Thus, trying to use the `AssemblyDefinition` in a "write" context will fail: var r = new DirectoryAssemblyResolver (...); var a = r.Load (assemblyName); a.Write (); // BOOM [0]: dotnet/java-interop#88

…nces Context: https://bugzilla.xamarin.com/show_bug.cgi?id=44529 Commit 1d65825 bumped to Cecil 0.10.0-beta1-v2 and Java.Interop/a1d3ecc8, which likewise uses Cecil 0.10.0-beta1-v2. Among the various API changes in Cecil 0.10.x vs. Cecil 0.9.x is a change to `Mono.Cecil.IAssemblyResolver`, implemented by `Java.Interop.Tools.Cecil.DirectoryAssemblyResolver`, to implement the `IDisposable` interface. Java.Interop/a1d3ecc8 added a `DirectoryAssemblyResolver.Dispose()` method, but this method didn't *do* anything. Cecil 0.10.0-beta1-v2 also has a *semantic* change vs. Cecil 0.9.x: `AssemblyDefinition` is no longer a "purely in-memory" construct. Instead, it holds a `System.IO.Stream` to the assembly it's reading (unless `Mono.Cecil.ReaderParameters.InMemory` is `true`). This semantic change means that an assembly can't be read from and written to at the same time via different mechanisms, so long as there is an `AssemblyDefinition` instance to that file. Which brings us to (private) Bug #44529, in which an `IOException` is thrown from the `<StripEmbeddedLibraries/>` task: Error executing task StripEmbeddedLibraries: System.IO.IOException: Sharing violation on path .../obj/Release/linksrc/Xamarin.Android.NUnitLite.dll at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share, System.Int32 bufferSize, System.Boolean anonymous, System.IO.FileOptions options) [0x0025f] in <253a3790b2c44512bbca8669ecfc1822>:0 at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share) [0x00000] in <253a3790b2c44512bbca8669ecfc1822>:0 at (wrapper remoting-invoke-with-check) System.IO.FileStream:.ctor (string,System.IO.FileMode,System.IO.FileAccess,System.IO.FileShare) at Mono.Cecil.ModuleDefinition.GetFileStream (System.String fileName, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share) [0x00007] in :0 at Mono.Cecil.ModuleDefinition.Write (System.String fileName, Mono.Cecil.WriterParameters parameters) [0x00007] in :0 at Mono.Cecil.AssemblyDefinition.Write (System.String fileName, Mono.Cecil.WriterParameters parameters) [0x00001] in :0 at Xamarin.Android.Tasks.StripEmbeddedLibraries.Execute () [0x0034a] in <3d5202a5d4874a76a99388021bf1ab1a>:0 The underlying cause of the `IOException` is twofold: 1. `AssemblyDefinition` instances now hold a `FileStream` to the file they're reading from (and optionally writing to), and this `FileStream` will be kept open until `AssemblyDefinition.Dispose()` is called. 2. Nothing in `Xamarin.Android.Build.Tasks` calls `AssemblyDefinition.Dispose()`. Nor should anything in `Xamarin.Android.Build.Tasks` call `AssemblyDefinition.Dispose()`! (Almost) All `AssemblyDefinition` instances are *cached* within `DirectoryAssemblyResolver` instances, and nothing calls `DirectoryAssemblyResolver.Dispose()` either! (Not that this matters *too* much, becuase `DirectoryAssemblyResolver.Dispose()` is currently a no-op.) There is no sane reasoning here: so long as we're using `DirectoryAssemblyResolver`, we're going to have more files open (compared to when Cecil 0.9.x was used), which can result in file sharing problems and the above `IOException`. The way foward is: 1. `Java.Interop.Tools.Cecil.DirectoryAssemblyResolver` needs to be updated so that [`DirectoryAssemblyResolver.Dispose()` is useful][0], i.e. that it will `Dispose()` of all held `AssemblyDefinition` instances. 2. Bump to Java.Interop/084e9f7f to pick up (1). 3. `Xamarin.Android.Build.Tasks.dll` needs to be updated so that `DirectoryAssemblyResolver.Dispose()` is used. 4. All code, in particular `Xamarin.Andorid.Build.Tasks.dll`, needs to be reviewed to ensure it doesn't do anything "stupid" with `AssemblyDefinition` instances and/or `DirectoryAssemblyResolver.` Examples of "stupid" things include: * `Dispose()`ing of the return value from `DirectoryAssemblyResolver.Load()`: resolver.Load (assemblyName).Dispose(); * Treating `AssemblyDefinition` instances in a manner inconsistent with how `DirectoryAssemblyResolver` loads them. `DirectoryAssemblyResolver` will be getting a new constructor overload which takes an `Mono.Cecil.ReaderParameters` parameter; the default will be the default Cecil behavior, which is for read-only behavior. Thus, trying to use the `AssemblyDefinition` in a "write" context will fail: var r = new DirectoryAssemblyResolver (...); var a = r.Load (assemblyName); a.Write (); // BOOM [0]: dotnet/java-interop#88

…nces (#233) Context: https://bugzilla.xamarin.com/show_bug.cgi?id=44529 Commit 1d65825 bumped to Cecil 0.10.0-beta1-v2 and Java.Interop/a1d3ecc8, which likewise uses Cecil 0.10.0-beta1-v2. Among the various API changes in Cecil 0.10.x vs. Cecil 0.9.x is a change to `Mono.Cecil.IAssemblyResolver`, implemented by `Java.Interop.Tools.Cecil.DirectoryAssemblyResolver`, to implement the `IDisposable` interface. Java.Interop/a1d3ecc8 added a `DirectoryAssemblyResolver.Dispose()` method, but this method didn't *do* anything. Cecil 0.10.0-beta1-v2 also has a *semantic* change vs. Cecil 0.9.x: `AssemblyDefinition` is no longer a "purely in-memory" construct. Instead, it holds a `System.IO.Stream` to the assembly it's reading (unless `Mono.Cecil.ReaderParameters.InMemory` is `true`). This semantic change means that an assembly can't be read from and written to at the same time via different mechanisms, so long as there is an `AssemblyDefinition` instance to that file. Which brings us to (private) Bug #44529, in which an `IOException` is thrown from the `<StripEmbeddedLibraries/>` task: Error executing task StripEmbeddedLibraries: System.IO.IOException: Sharing violation on path .../obj/Release/linksrc/Xamarin.Android.NUnitLite.dll at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share, System.Int32 bufferSize, System.Boolean anonymous, System.IO.FileOptions options) [0x0025f] in <253a3790b2c44512bbca8669ecfc1822>:0 at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share) [0x00000] in <253a3790b2c44512bbca8669ecfc1822>:0 at (wrapper remoting-invoke-with-check) System.IO.FileStream:.ctor (string,System.IO.FileMode,System.IO.FileAccess,System.IO.FileShare) at Mono.Cecil.ModuleDefinition.GetFileStream (System.String fileName, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share) [0x00007] in :0 at Mono.Cecil.ModuleDefinition.Write (System.String fileName, Mono.Cecil.WriterParameters parameters) [0x00007] in :0 at Mono.Cecil.AssemblyDefinition.Write (System.String fileName, Mono.Cecil.WriterParameters parameters) [0x00001] in :0 at Xamarin.Android.Tasks.StripEmbeddedLibraries.Execute () [0x0034a] in <3d5202a5d4874a76a99388021bf1ab1a>:0 The underlying cause of the `IOException` is twofold: 1. `AssemblyDefinition` instances now hold a `FileStream` to the file they're reading from (and optionally writing to), and this `FileStream` will be kept open until `AssemblyDefinition.Dispose()` is called. 2. Nothing in `Xamarin.Android.Build.Tasks` calls `AssemblyDefinition.Dispose()`. Nor should anything in `Xamarin.Android.Build.Tasks` call `AssemblyDefinition.Dispose()`! (Almost) All `AssemblyDefinition` instances are *cached* within `DirectoryAssemblyResolver` instances, and nothing calls `DirectoryAssemblyResolver.Dispose()` either! (Not that this matters *too* much, becuase `DirectoryAssemblyResolver.Dispose()` is currently a no-op.) There is no sane reasoning here: so long as we're using `DirectoryAssemblyResolver`, we're going to have more files open (compared to when Cecil 0.9.x was used), which can result in file sharing problems and the above `IOException`. The way foward is: 1. `Java.Interop.Tools.Cecil.DirectoryAssemblyResolver` needs to be updated so that [`DirectoryAssemblyResolver.Dispose()` is useful][0], i.e. that it will `Dispose()` of all held `AssemblyDefinition` instances. 2. Bump to Java.Interop/084e9f7f to pick up (1). 3. `Xamarin.Android.Build.Tasks.dll` needs to be updated so that `DirectoryAssemblyResolver.Dispose()` is used. 4. All code, in particular `Xamarin.Andorid.Build.Tasks.dll`, needs to be reviewed to ensure it doesn't do anything "stupid" with `AssemblyDefinition` instances and/or `DirectoryAssemblyResolver.` Examples of "stupid" things include: * `Dispose()`ing of the return value from `DirectoryAssemblyResolver.Load()`: resolver.Load (assemblyName).Dispose(); * Treating `AssemblyDefinition` instances in a manner inconsistent with how `DirectoryAssemblyResolver` loads them. `DirectoryAssemblyResolver` will be getting a new constructor overload which takes an `Mono.Cecil.ReaderParameters` parameter; the default will be the default Cecil behavior, which is for read-only behavior. Thus, trying to use the `AssemblyDefinition` in a "write" context will fail: var r = new DirectoryAssemblyResolver (...); var a = r.Load (assemblyName); a.Write (); // BOOM [0]: dotnet/java-interop#88

Context: #87 (comment) Context: https://bugzilla.xamarin.com/show_bug.cgi?id=44529 (Private) Bug #44529 is an `IOException` on `xamarin-android/master` due to file sharing: Error executing task StripEmbeddedLibraries: System.IO.IOException: Sharing violation on path .../obj/Release/linksrc/Xamarin.Android.NUnitLite.dll at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share, System.Int32 bufferSize, System.Boolean anonymous, System.IO.FileOptions options) [0x0025f] in <253a3790b2c44512bbca8669ecfc1822>:0 at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share) [0x00000] in <253a3790b2c44512bbca8669ecfc1822>:0 at (wrapper remoting-invoke-with-check) System.IO.FileStream:.ctor (string,System.IO.FileMode,System.IO.FileAccess,System.IO.FileShare) at Mono.Cecil.ModuleDefinition.GetFileStream (System.String fileName, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share) [0x00007] in :0 at Mono.Cecil.ModuleDefinition.Write (System.String fileName, Mono.Cecil.WriterParameters parameters) [0x00007] in :0 at Mono.Cecil.AssemblyDefinition.Write (System.String fileName, Mono.Cecil.WriterParameters parameters) [0x00001] in :0 at Xamarin.Android.Tasks.StripEmbeddedLibraries.Execute () [0x0034a] in <3d5202a5d4874a76a99388021bf1ab1a>:0 The underlying cause of this change is the migration to Cecil 0.10.0-beta1-v2 (dfed286), which -- along with API changes -- has some *semantic* changes [^0]. In particular, within Cecil <= 0.9.x, `AssemblyDefinition` was entirely in-memory. Starting with Cecil 0.10.x, `AssemblyDefinition` isn't; it is backed by a `System.IO.Stream`, which can be in-memory (if `ReaderParameters.InMemory` is `true`), or a `FileStream` (the default). This normally might not be bad, except we also have `Java.Interop.Tools.Cecil.DirectoryAssemblyResolver`, which *caches* all created `AssemblyDefinition` instances. Thus, "normal" *correct* Cecil use would be fine, so long as you know all assemblies which have been loaded, load them with the correct `ReaderParameters`, and promptly `Dispose()` of the `AssemblyDefinition` when done. `DirectoryAssemblyResolver` throws a wrench in that, because (1) commit dfed286 incorrectly implemented `DirectoryAssemblyResolver.Dispose()`, leaving all of the cached `AssemblyDefinition` instances still "live", which means (2) The lifetime of the `Stream` underlying the `AssemblyDefinition` is controlled by the GC, which can mean nearly anything. This is all a *huge* recipe for confusion. Fix `DirectoryAssemblyResolver.Dispose()` so that the cached `AssemblyDefinition` instances are `Dispose()`d of, and review all use of `DirectoryAssemblyResolver` within Java.Interop to ensure that any created instances are appropriate `Dispose()`d of. Additionally, add a new `DirectoryAssemblyResolver` constructor to allow controlling the `ReaderParameters` that `DirectoryAssemblyResolver.Load()` uses when loading an assembly: partial class DirectoryAssemblyResolver { public DirectoryAssemblyResolver ( Action<string, object[]> logWarnings, bool loadDebugSymbols, ReaderParameters loadReaderParameters = null); } The new `loadReaderParameters` allows specifying the default `ReaderParameters` values to use when `DirectoryAssemblyResolver.Load()` is invoked. This ensures that all assemblies loaded by `DirectoryAssemblyResolver` are loaded in a consistent fashion (e.g. readonly, read+write, in-memory), which will hopefully allow code to be sanely reasoned about. [^0]: The best kind of changes!

Changes: dotnet/android-tools@f5fcb9f...3974fc3 * dotnet/android-tools@3974fc3: [Xamarin.Android.Tools.AndroidSdk] JdkInfo + JDK11 + Windows (dotnet#88) * dotnet/android-tools@5552b07: [Xamarin.Android.Tools.AndroidSdk] Improve utility of JDK warnings (dotnet#87) * dotnet/android-tools@13cc497: [Xamarin.Android.Tools.AndroidSdk] Prefer JAVA_HOME (dotnet#86) * dotnet/android-tools@967c278: Delete NuGet.Config * dotnet/android-tools@2d3690e: [Xamarin.Android.Tools.AndroidSdk] Nullable Reference Type support (dotnet#84) * dotnet/android-tools@2020b20: [Xamarin.Android.Tools.AndroidSdk] Preview Build-Tools are Last (dotnet#85)

Changes: dotnet/android-tools@f5fcb9f...3974fc3 * dotnet/android-tools@3974fc3: [Xamarin.Android.Tools.AndroidSdk] JdkInfo + JDK11 + Windows (#88) * dotnet/android-tools@5552b07: [Xamarin.Android.Tools.AndroidSdk] Improve utility of JDK warnings (#87) * dotnet/android-tools@13cc497: [Xamarin.Android.Tools.AndroidSdk] Prefer JAVA_HOME (#86) * dotnet/android-tools@967c278: Delete NuGet.Config * dotnet/android-tools@2d3690e: [Xamarin.Android.Tools.AndroidSdk] Nullable Reference Type support (#84) * dotnet/android-tools@2020b20: [Xamarin.Android.Tools.AndroidSdk] Preview Build-Tools are Last (#85)

Changes: dotnet/android-tools@23c4fe0...017078f * dotnet/android-tools@017078f: [Xamarin.Android.Tools.AndroidSdk] JdkInfo + JDK11 + Windows (#88) * dotnet/android-tools@852e4a3: [Xamarin.Android.Tools.AndroidSdk] Improve utility of JDK warnings (#87) * dotnet/android-tools@b055edf: [Xamarin.Android.Tools.AndroidSdk] Prefer JI_JAVA_HOME (#86) * dotnet/android-tools@ef31658: [Xamarin.Android.Tools.AndroidSdk] Nullable Reference Type support (#84) * dotnet/android-tools@b8efdae: [Xamarin.Android.Tools.AndroidSdk] Preview Build-Tools are Last (#85)

dnfclas added the cla-already-signed label Sep 23, 2016

jonpryor mentioned this pull request Sep 23, 2016

[DirectoryAssemblyResolver] add GetAssembly () that allows to set R… #87

Closed

marek-safar reviewed Sep 23, 2016

View reviewed changes

lewurm reviewed Sep 23, 2016

View reviewed changes

jonpryor force-pushed the jonp-resolver branch 3 times, most recently from 6828bdb to 89931de Compare September 23, 2016 18:44

jonpryor force-pushed the jonp-resolver branch from 89931de to 1c13c35 Compare September 23, 2016 18:48

This was referenced Sep 24, 2016

[Xamarin.Android.Build.Tasks] Dispose DirectoryAssemblyResolver instances dotnet/android#233

Merged

[Xamarin.Android.Build.Tasks] make sure Disposable () is called for assembly dotnet/android#231

Closed

atsushieno merged commit 084e9f7 into dotnet:master Sep 26, 2016

jonpryor mentioned this pull request Jun 4, 2020

Bump to xamarin/xamarin-android-tools/master@3974fc38 #658

Merged

github-actions bot locked and limited conversation to collaborators Apr 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Java.Interop.Tools.Cecil] Fix DirectoryAssemblyResolver.Dispose() #88

[Java.Interop.Tools.Cecil] Fix DirectoryAssemblyResolver.Dispose() #88

jonpryor commented Sep 23, 2016

marek-safar Sep 23, 2016

jonpryor Sep 25, 2016

marek-safar Sep 23, 2016

jonpryor Sep 23, 2016

marek-safar Sep 23, 2016

jonpryor Sep 23, 2016

marek-safar Sep 23, 2016

jonpryor Sep 23, 2016

marek-safar Sep 25, 2016

jonpryor Sep 25, 2016

marek-safar Sep 25, 2016

atsushieno Sep 26, 2016

lewurm Sep 23, 2016

jonpryor Sep 23, 2016

jonpryor Sep 23, 2016

lewurm commented Sep 23, 2016

[Java.Interop.Tools.Cecil] Fix DirectoryAssemblyResolver.Dispose() #88

[Java.Interop.Tools.Cecil] Fix DirectoryAssemblyResolver.Dispose() #88

Conversation

jonpryor commented Sep 23, 2016

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lewurm commented Sep 23, 2016