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

[lewurm]

…eaderParameters and fix Resolve () so that it actually uses parameters

[dnfclas]

Hi @lewurm, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

[dellis1972]

@lewurm did you miss adding 'ReaderParameters parameters' on line 147?
also we can make use of
public virtual AssemblyDefinition Load (string fileName, ReaderParameters parameters = null)
so we don't need to duplicate the Load method same for ReadAssembly.

[lewurm]

no, there's already a version with ReaderParameters on line 152.
thanks for the hint about default parameter values, I'll change that.

[jonpryor]

TL;DR: No.

What I didn't realize is that this PR is related to Bug 44529, which dotnet/android#231 attempts to address.

Background: In the "old times" (Cecil <= 0.9.x), AssemblyDefinition was an in-memory construct. It did not implement IDisposable, and calling code could (more or less) call AssemblyDefinition.ReadAssembly() with impunity, because everything would be kept in memory; there couldn't be any issues with file sharing exceptions.

It was in this world order that DirectoryAssemblyResolver was originally written.

Then came the Cecil 0.10 change, which completely alters these semantics: AssemblyDefinition now implements IDisposable, and also keeps open a file stream. Additionally, if ReaderParameters.ReadWrite is true, the assembly can only be opened once:

var rp = new ReaderParameters () { ReadWrite = true };
var r1 = AssemblyDefinition.ReadAssembly ("Mono.Cecil.dll", rp);
var r2 = AssemblyDefinition.ReadAssembly ("Mono.Cecil.dll", rp);
// System.IO.IOException: Sharing violation on path .../Mono.Cecil.dll

What does DirectoryAssemblyResolver do? It implements IAssemblyResolver, yes, but it also caches all resolved assemblies. DirectoryAssemblyResolver.Resolve()/etc. not only loads the assembly and returns an AssemblyDefinition, but it caches that AssemblyDefinition instance so that future calls to .Resolve()/etc. with the same assembly name return the same instance.

Very important aside: Commit dfed286 is buggy, because DirectoryAssemblyResolver.Dispose(bool) is a no-op. It needs to be corrected to also call AssemblyDefinition.Dispose() on all cached instances.

This caching means that -- by design! -- DirectoryAssemblyResolver will introduce AssemblyDefinition sharing/reuse. Meaning this code would be Bad™:

var r = new DirectoryAssemblyResolver (...);
using (var a = r.Load (filename)) {
    // ...
}
var a2 = r.Load (filename);
// a2 contains a *disposed* AssemblyDefinition instance.

Fortunately, there should be no such code in Java.Interop or xamarin-android, because AssemblyDefinition didn't previously implement IDisposable, so it wouldn't be possible to do using (var a = r.Load(filename)).

Which allows me to breath slightly easier. (Oh the panic attack before I realized that...)

Let us now return to the IOException and DirectoryAssemblyResolver. We should not provide any DirectoryAssemblyResolver methods which take ReaderParameters as a parameter. Why not? Because DirectoryAssemblyResolver caches the AssemblyDefinitions, and if different AssemblyDefinitions had different associated ReaderParameters, I'm not even sure how to begin reasoning about the resulting code:

var r = new DirectoryAssemblyResolver (...);
r.Load ("Mono.Cecil.dll");
r.GetAssembly ("Mono.Cecil.dll", new ReaderParameters {...});
// ReaderParameters *ignored*; previously loaded `Mono.Cecil.dll` returned

This is particularly difficult, considering that IAssemblyResolver -- and thus DirectoryAssemblyResolver -- is implicitly used when trying to resolve types, e.g. in the linker/etc. It's very easy to get very confused.

---

Proposal:

In order to not go crazy, I believe the following needs to happen:

1. DirectoryAssemblyResolver.Dispose(bool) needs to be fixed so that all cached AssemblyDefinition instances are disposed of:

   protected virtual void Dispose (bool disposing)
   {
       if (!disposing || cache == null)
           return;
       foreach (var e in cache) {
           e.Value.Dispose ();
       }
       cache = null;
   }
   

2. The DirectoryAssemblyResolver constructor should be updated to accept a "template" ReaderParameters instance. The semantics would thus be that all (almost all?) DirectoryAssemblyResolver.Load()/etc. would use the ReaderParameters instance provided to the constructor.

   Because DirectoryAssemblyResolver.Resolve() uses Load(), and Resolve() is implicitly used during e.g. type resolution, this would allow controlling all assembly resolution to e.g. be read+write, instead of read-only (the default).

3. All use of DirectoryAssemblyResolver, everywhere, needs to be audited to ensure:

   1. That the DirectoryAssemblyResolver instance is Dispose()d of.
   2. That use of Load()/Resolve()/GetAssembly() is consistent with the caching semantics of DirectoryAssemblyResolver, e.g. no calling GetAssembly("Foo").Dispose().

[jonpryor]

Superseded by RP #88.