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

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!

Author: jonpryor

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

@@ -64,14 +64,20 @@ public class DirectoryAssemblyResolver : IAssemblyResolver {
 		bool loadDebugSymbols;
 		Action<string, object[]> logWarnings;
 
-		public DirectoryAssemblyResolver (Action<string, object[]> logWarnings, bool loadDebugSymbols)
+		ReaderParameters                        loadReaderParameters;
+
+		static  readonly    ReaderParameters    DefaultLoadReaderParameters = new ReaderParameters {
+		};
+
+		public DirectoryAssemblyResolver (Action<string, object[]> logWarnings, bool loadDebugSymbols, ReaderParameters loadReaderParameters = null)
 		{
 			if (logWarnings == null)
 				throw new ArgumentNullException (nameof (logWarnings));
 			cache = new Dictionary<string, AssemblyDefinition> ();
 			this.loadDebugSymbols = loadDebugSymbols;
 			this.logWarnings = logWarnings;
 			SearchDirectories = new List<string> ();
+			this.loadReaderParameters = loadReaderParameters ?? DefaultLoadReaderParameters;
 		}
 
 		public void Dispose ()
@@ -82,8 +88,15 @@ public void Dispose ()
 
 		protected virtual void Dispose (bool disposing)
 		{
+			if (!disposing || cache == null)
+				return;
+			foreach (var e in cache) {
+				e.Value.Dispose ();
+			}
+			cache = null;
 		}
 
+		[Obsolete ("Should not be used; was required with previous Cecil versions.")]
 		public IDictionary ToResolverCache ()
 		{
 			var resolver_cache = new Hashtable ();
@@ -114,9 +127,21 @@ public virtual AssemblyDefinition Load (string fileName)
 
 		protected virtual AssemblyDefinition ReadAssembly (string file)
 		{
+			bool haveDebugSymbols = loadDebugSymbols &&
+				(File.Exists (file + ".mdb") ||
+				 File.Exists (Path.ChangeExtension (file, ".pdb")));
 			var reader_parameters = new ReaderParameters () {
-				AssemblyResolver  = this,
-				ReadSymbols       = loadDebugSymbols && (File.Exists(file + ".mdb") || File.Exists(Path.ChangeExtension(file, ".pdb"))),
+				ApplyWindowsRuntimeProjections  = loadReaderParameters.ApplyWindowsRuntimeProjections,
+				AssemblyResolver                = this,
+				MetadataImporterProvider        = loadReaderParameters.MetadataImporterProvider,
+				InMemory                        = loadReaderParameters.InMemory,
+				MetadataResolver                = loadReaderParameters.MetadataResolver,
+				ReadingMode                     = loadReaderParameters.ReadingMode,
+				ReadSymbols                     = haveDebugSymbols,
+				ReadWrite                       = loadReaderParameters.ReadWrite,
+				ReflectionImporterProvider      = loadReaderParameters.ReflectionImporterProvider,
+				SymbolReaderProvider            = loadReaderParameters.SymbolReaderProvider,
+				SymbolStream                    = loadReaderParameters.SymbolStream,
 			};
 			try {
 				return AssemblyDefinition.ReadAssembly (file, reader_parameters);

src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/TypeNameMapGenerator.cs

@@ -68,13 +68,15 @@ public TypeNameMapGenerator (IEnumerable<string> assemblies, Action<string, obje
 
 			Log             = logMessage;
 			var Assemblies  = assemblies.ToList ();
-			var resolver    = new DirectoryAssemblyResolver (Log, loadDebugSymbols: true);
+			var rp          = new ReaderParameters (ReadingMode.Immediate);
 
-			foreach (var assembly in Assemblies) {
-				resolver.Load (Path.GetFullPath (assembly));
-			}
+			using (var resolver = new DirectoryAssemblyResolver (Log, loadDebugSymbols: true, loadReaderParameters: rp)) {
+				foreach (var assembly in Assemblies) {
+					resolver.Load (Path.GetFullPath (assembly));
+				}
 
-			Types       = JavaTypeScanner.GetJavaTypes (Assemblies, resolver, logMessage);
+				Types       = JavaTypeScanner.GetJavaTypes (Assemblies, resolver, logMessage);
+			}
 		}
 
 		public TypeNameMapGenerator (IEnumerable<TypeDefinition> types, Action<string, object[]> logMessage)

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 ();
 				return;
+			}
 
 			// load XML API definition with fixups.
 
@@ -296,6 +298,7 @@ public static void Run (CodeGeneratorOptions options)
 			Parser p = new Parser ();
 			List<GenBase> gens = p.Parse (apiXmlFile, fixups, api_level, product_version);
 			if (gens == null) {
+				resolver.Dispose ();
 				return;
 			}
 			apiSource = p.ApiSource;
@@ -347,6 +350,8 @@ public static void Run (CodeGeneratorOptions options)
 				: enummap.WriteEnumerations (enumdir, enums, FlattenNestedTypes (gens).ToArray (), opt.UseShortFileNames);
 
 			gen_info.GenerateLibraryProjectFile (options, enumFiles);
+
+			resolver.Dispose ();
 		}
 
 		static IEnumerable<GenBase> FlattenNestedTypes (IEnumerable<GenBase> gens)

tools/jcw-gen/App.cs

@@ -48,8 +48,7 @@ public static int Main (string [] args)
 					if (assemblies.Count == 0) {
 						Console.Error.WriteLine ("jcw-gen: No assemblies specified.");
 						r = 1;
-					}
-					else if (outputPath == null) {
+					} else if (outputPath == null) {
 						Console.Error.WriteLine ("jcw-gen: No output directory specified. Use `jcw-gen -o PATH`.");
 						r = 1;
 					}
@@ -66,10 +65,11 @@ public static int Main (string [] args)
 					GenerateJavaCallableWrapper (type, outputPath);
 				}
 				return 0;
-			}
-			catch (Exception e) {
+			} catch (Exception e) {
 				Console.Error.Write ("jcw-gen: {0}", verbosity > 0 ? e.ToString () : e.Message);
 				return 1;
+			} finally {
+				resolver.Dispose ();
 			}
 		}