From 1c13c357db14eca204af1cde456d200002e9861f Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Fri, 23 Sep 2016 12:33:42 -0400 Subject: [PATCH] [Java.Interop.Tools.Cecil] Fix DirectoryAssemblyResolver.Dispose() Context: https://github.com/xamarin/java.interop/pull/87#issuecomment-249227731 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 (dfed286d), 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 dfed286d 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 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! --- .../DirectoryAssemblyResolver.cs | 31 +++++++++++++++++-- .../TypeNameMapGenerator.cs | 12 ++++--- tools/generator/CodeGenerator.cs | 8 ++++- tools/jcw-gen/App.cs | 3 ++ 4 files changed, 45 insertions(+), 9 deletions(-) diff --git a/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs b/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs index 7068c4112..d2a41d9d0 100644 --- a/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs +++ b/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs @@ -64,7 +64,12 @@ public class DirectoryAssemblyResolver : IAssemblyResolver { bool loadDebugSymbols; Action logWarnings; - public DirectoryAssemblyResolver (Action logWarnings, bool loadDebugSymbols) + ReaderParameters loadReaderParameters; + + static readonly ReaderParameters DefaultLoadReaderParameters = new ReaderParameters { + }; + + public DirectoryAssemblyResolver (Action logWarnings, bool loadDebugSymbols, ReaderParameters loadReaderParameters = null) { if (logWarnings == null) throw new ArgumentNullException (nameof (logWarnings)); @@ -72,6 +77,7 @@ public DirectoryAssemblyResolver (Action logWarnings, bool loa this.loadDebugSymbols = loadDebugSymbols; this.logWarnings = logWarnings; SearchDirectories = new List (); + 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); diff --git a/src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/TypeNameMapGenerator.cs b/src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/TypeNameMapGenerator.cs index 188aba9f5..a39775135 100644 --- a/src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/TypeNameMapGenerator.cs +++ b/src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers/TypeNameMapGenerator.cs @@ -68,13 +68,15 @@ public TypeNameMapGenerator (IEnumerable assemblies, Action types, Action logMessage) diff --git a/tools/generator/CodeGenerator.cs b/tools/generator/CodeGenerator.cs index 320863cf6..2a9760a29 100644 --- a/tools/generator/CodeGenerator.cs +++ b/tools/generator/CodeGenerator.cs @@ -209,6 +209,13 @@ public static void Run (CodeGeneratorOptions options) if (options == null) throw new ArgumentNullException ("options"); + using (var resolver = new DirectoryAssemblyResolver (Console.WriteLine, loadDebugSymbols: false)) { + Run (options, resolver); + } + } + + static void Run (CodeGeneratorOptions options, DirectoryAssemblyResolver resolver) + { string assemblyQN = options.AssemblyQualifiedName; string api_level = options.ApiLevel; int product_version = options.ProductVersion; @@ -239,7 +246,6 @@ public static void Run (CodeGeneratorOptions options) // Load reference libraries - var resolver = new DirectoryAssemblyResolver (Console.WriteLine, loadDebugSymbols: false); foreach (var lib in options.LibraryPaths) { resolver.SearchDirectories.Add (lib); } diff --git a/tools/jcw-gen/App.cs b/tools/jcw-gen/App.cs index 1f263c698..8de3ac526 100644 --- a/tools/jcw-gen/App.cs +++ b/tools/jcw-gen/App.cs @@ -71,6 +71,9 @@ public static int Main (string [] args) Console.Error.Write ("jcw-gen: {0}", verbosity > 0 ? e.ToString () : e.Message); return 1; } + finally { + resolver.Dispose (); + } } static void GenerateJavaCallableWrapper (TypeDefinition type, string outputPath)