LoadFromResolve event handler should return null #12329
Conversation
|
LGTM. just wondering, there is no way to prob loading the assembly without throwing at all instead of throwing and then catching the exception? |
|
It is not just about probing the presence but for any load error. Essentially, AssemblyResolve does not expect anything but valid assembly or null from its callbacks, per the documentation. |
Yes I understand this. I am asking if we have any internal API similar to Assembly.LoadFrom which doesn't throw at all. all my point is I am trying to see if there is a way to avoid throwing and then catching for the sake of the performance and also to avoid the debuggers break on such handled exceptions. I am ok with the change, I am just asking if we have anyway to do that :-) |
|
The documentation, however, is different from implementation - https://github.com/dotnet/coreclr/blob/master/src/vm/appdomain.cpp#L7090 :( |
|
Callstack at which exception is thrown in LoadFromResolve event handler: |
|
@tarekgh The implementation indicates that managed resolve event handlers can throw and their exceptions need to be propagated (which is what is happening in the bug). If we go via that logic, the fix should be in one of the following frames (perhaps 1b since it already has try/catch for other scenarios when failing to resolve satellite assemblies): What do you and @jkotas think about this? |
|
Looking at https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Resources/ManifestBasedResourceGroveler.cs#L426, it seems to me that the fix should be in ManifestBasedResourceGroveler. Would you both agree since it allows the exception propagation, as expected, and also line up with the scenarios that function handles? |
When the |
It can throw exceptions like out of memory, and those should propagate. If it cannot find what it is asked to find, it should return null - not throw. |
|
one more question regarding the method nLoad This method takes a parameter throwOnFileNotFound, and I think it should respect this parameter but this is not happening. I am seeing nLoad is called in 2 places, the first is from InternalGetSatelliteAssembly and I think your fix will address that. But I am seeing it is called from InternalLoadAssemblyName too and I think there will be a problem there for anyone calling it with throwOnFileNotFound=false and then nLoad eventually throw FileNotFound |
The documentation does not indicate so - it simply says valid or null reference back. That said, if there was another handler registered for AssemblyResolve, prior to LoadFrom, and it throws an exception, we will landup with this issue. |
|
@tarekgh That parameter gets respected unless we are in the fallback mode to invoke AssemblyResolve event against AppDomain (which is what is happening in this issue). |
Is it possible InternalLoadAssemblyName get called in the same fallback mode? if so then this code path has the same problem similar to what we are fixing here. I maybe missing something here. |
Exception for fatal errors can be thrown anywhere. The documentation does not explicitly mentions them. It think it would be better for this to catch |
|
@jkotas I was thinking about it more and I agree - FNF maps to missing file and is okay to be returned as null. Everything else should get propagated. I will make the change. |
gkhanna79
force-pushed
the
LoadFromResExMaster
branch
from
6eea6a1
to
91a2e69
Compare
|
@jkotas Updated to catch only FNF. |
|
@dotnet-bot test OSX10.12 x64 Checked Build and Test please |
|
@mmitche Is the OSX failure a known issue (see https://ci.dot.net/job/dotnet_coreclr/job/master/job/checked_osx10.12_flow_prtest/4079/consoleFull#-1637969066ee26338-6221-4f7c-8c8f-1d1d5cb4704a):
|
|
@dotnet-bot test OSX10.12 x64 Checked Build and Test please |
|
@gkhanna79 Network issues |
|
@mmitche Is this latest attempt to rerun the leg also an infra issue? |
|
cc @dotnet/dnceng You can take the offending machine offline, otherwise they tend to eat runs for a few mins. |
|
@dotnet-bot test OSX10.12 x64 Checked Build and Test |
Since LoadFromResolve event handler is wired upto AppDomain.AssemblyResolve event, it should return null when an assembly cannot be found for any reason as AssemblyResolve event expects that (See https://msdn.microsoft.com/en-us/library/system.appdomain.assemblyresolve(v=vs.110).aspx).
Fixes dotnet/corefx#21095
@jkotas @tarekgh PTAL