Commit 712be22
committed
[Xamarin.Android.Build.Tasks] Dispose DirectoryAssemblyResolver instances
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#881 parent 9187b4d commit 712be22
File tree
9 files changed
+120
-96
lines changed- src/Xamarin.Android.Build.Tasks/Tasks
9 files changed
+120
-96
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
444 | 444 | | |
445 | 445 | | |
446 | 446 | | |
447 | | - | |
| 447 | + | |
448 | 448 | | |
449 | 449 | | |
450 | | - | |
451 | | - | |
452 | | - | |
453 | | - | |
454 | | - | |
455 | | - | |
456 | | - | |
457 | | - | |
458 | | - | |
459 | | - | |
460 | | - | |
461 | | - | |
462 | | - | |
463 | | - | |
464 | | - | |
465 | | - | |
466 | | - | |
467 | | - | |
468 | | - | |
469 | | - | |
470 | | - | |
| 450 | + | |
| 451 | + | |
| 452 | + | |
| 453 | + | |
| 454 | + | |
| 455 | + | |
| 456 | + | |
| 457 | + | |
| 458 | + | |
| 459 | + | |
| 460 | + | |
| 461 | + | |
| 462 | + | |
| 463 | + | |
| 464 | + | |
| 465 | + | |
| 466 | + | |
| 467 | + | |
| 468 | + | |
| 469 | + | |
| 470 | + | |
| 471 | + | |
471 | 472 | | |
472 | 473 | | |
473 | 474 | | |
| |||
Lines changed: 9 additions & 9 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
23 | 23 | | |
24 | 24 | | |
25 | 25 | | |
26 | | - | |
27 | 26 | | |
28 | | - | |
| 27 | + | |
29 | 28 | | |
30 | 29 | | |
31 | 30 | | |
| |||
51 | 50 | | |
52 | 51 | | |
53 | 52 | | |
54 | | - | |
55 | | - | |
56 | | - | |
57 | | - | |
58 | | - | |
59 | | - | |
60 | | - | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
61 | 61 | | |
62 | 62 | | |
63 | 63 | | |
| |||
Lines changed: 6 additions & 6 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
79 | 79 | | |
80 | 80 | | |
81 | 81 | | |
82 | | - | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
83 | 87 | | |
84 | 88 | | |
85 | 89 | | |
| |||
90 | 94 | | |
91 | 95 | | |
92 | 96 | | |
93 | | - | |
| 97 | + | |
94 | 98 | | |
95 | 99 | | |
96 | 100 | | |
97 | 101 | | |
98 | 102 | | |
99 | 103 | | |
100 | | - | |
101 | | - | |
102 | | - | |
103 | | - | |
104 | 104 | | |
105 | 105 | | |
106 | 106 | | |
| |||
Lines changed: 17 additions & 16 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
106 | 106 | | |
107 | 107 | | |
108 | 108 | | |
109 | | - | |
110 | | - | |
111 | | - | |
112 | | - | |
113 | | - | |
114 | | - | |
115 | | - | |
116 | | - | |
117 | | - | |
118 | | - | |
119 | | - | |
120 | | - | |
121 | | - | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
122 | 126 | | |
123 | | - | |
124 | | - | |
125 | | - | |
126 | 127 | | |
127 | 128 | | |
128 | 129 | | |
| |||
Lines changed: 23 additions & 22 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
363 | 363 | | |
364 | 364 | | |
365 | 365 | | |
366 | | - | |
367 | | - | |
368 | | - | |
369 | | - | |
370 | | - | |
371 | | - | |
372 | | - | |
373 | | - | |
374 | | - | |
375 | | - | |
376 | | - | |
377 | | - | |
378 | | - | |
379 | | - | |
380 | | - | |
381 | | - | |
382 | | - | |
383 | | - | |
384 | | - | |
385 | | - | |
386 | | - | |
387 | | - | |
| 366 | + | |
| 367 | + | |
| 368 | + | |
| 369 | + | |
| 370 | + | |
| 371 | + | |
| 372 | + | |
| 373 | + | |
| 374 | + | |
| 375 | + | |
| 376 | + | |
| 377 | + | |
| 378 | + | |
| 379 | + | |
| 380 | + | |
| 381 | + | |
| 382 | + | |
| 383 | + | |
| 384 | + | |
| 385 | + | |
| 386 | + | |
| 387 | + | |
| 388 | + | |
388 | 389 | | |
389 | 390 | | |
390 | 391 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
75 | 75 | | |
76 | 76 | | |
77 | 77 | | |
78 | | - | |
79 | | - | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
80 | 88 | | |
81 | 89 | | |
82 | 90 | | |
| |||
Lines changed: 20 additions & 16 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
42 | 42 | | |
43 | 43 | | |
44 | 44 | | |
45 | | - | |
46 | | - | |
47 | 45 | | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
48 | 53 | | |
49 | 54 | | |
50 | 55 | | |
51 | 56 | | |
52 | 57 | | |
53 | 58 | | |
54 | 59 | | |
55 | | - | |
56 | 60 | | |
57 | 61 | | |
58 | 62 | | |
| |||
80 | 84 | | |
81 | 85 | | |
82 | 86 | | |
83 | | - | |
| 87 | + | |
84 | 88 | | |
85 | 89 | | |
86 | 90 | | |
87 | 91 | | |
88 | 92 | | |
89 | 93 | | |
90 | | - | |
| 94 | + | |
91 | 95 | | |
92 | 96 | | |
93 | 97 | | |
| |||
100 | 104 | | |
101 | 105 | | |
102 | 106 | | |
103 | | - | |
| 107 | + | |
104 | 108 | | |
105 | 109 | | |
106 | 110 | | |
107 | 111 | | |
108 | 112 | | |
109 | | - | |
| 113 | + | |
110 | 114 | | |
111 | 115 | | |
112 | 116 | | |
| |||
130 | 134 | | |
131 | 135 | | |
132 | 136 | | |
133 | | - | |
| 137 | + | |
134 | 138 | | |
135 | 139 | | |
136 | 140 | | |
| |||
147 | 151 | | |
148 | 152 | | |
149 | 153 | | |
150 | | - | |
| 154 | + | |
151 | 155 | | |
152 | 156 | | |
153 | 157 | | |
| |||
156 | 160 | | |
157 | 161 | | |
158 | 162 | | |
159 | | - | |
| 163 | + | |
160 | 164 | | |
161 | 165 | | |
162 | | - | |
| 166 | + | |
163 | 167 | | |
164 | 168 | | |
165 | | - | |
| 169 | + | |
166 | 170 | | |
167 | 171 | | |
168 | | - | |
| 172 | + | |
169 | 173 | | |
170 | 174 | | |
171 | | - | |
| 175 | + | |
172 | 176 | | |
173 | 177 | | |
174 | | - | |
| 178 | + | |
175 | 179 | | |
176 | 180 | | |
177 | | - | |
| 181 | + | |
178 | 182 | | |
179 | 183 | | |
180 | 184 | | |
| |||
Lines changed: 5 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
71 | 71 | | |
72 | 72 | | |
73 | 73 | | |
74 | | - | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
75 | 78 | | |
76 | 79 | | |
77 | 80 | | |
| |||
133 | 136 | | |
134 | 137 | | |
135 | 138 | | |
| 139 | + | |
136 | 140 | | |
137 | 141 | | |
138 | 142 | | |
| |||
142 | 146 | | |
143 | 147 | | |
144 | 148 | | |
145 | | - | |
146 | 149 | | |
147 | 150 | | |
148 | 151 | | |
| |||
Lines changed: 7 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
28 | 28 | | |
29 | 29 | | |
30 | 30 | | |
31 | | - | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
32 | 38 | | |
33 | 39 | | |
34 | 40 | | |
| |||
0 commit comments