U4-8652 - ReflectionTypeLoadExceptions in WebBootManager

Created by Roelof Nijholt 23 Jun 2016, 09:26:53 Updated by Sebastiaan Janssen 24 Oct 2017, 07:34:29

Tags: Unscheduled PR

Subtask of: U4-9609

What happened? When 2 external libraries have references to different versions of the same third party library, and that library has changed (e.g. namespace changes), the Umbraco WebBootManager can throw a breaking ReflectionTypeLoadException.

What did you expect to happen? Hopefully continue running, as the issue would not necessarily be breaking within the project. The conflicting Type could be skipped by Umbraco. (proposed fix on Github: https://github.com/umbraco/Umbraco-CMS/pull/1349)

What actually happened? ReflectionTypeLoadException thrown, boot halted

1 Attachments

Download TestLib.dll

Comments

Stephan 23 Jun 2016, 12:37:13

Copying from the PR so we can have the discussion here... my remark was:

I guess this enables Umbraco to run even though the whole DLL set in ~/bin is not fully consistent, ie some dependencies cannot be resolved. Unresolved dependencies can break many things, including dynamics extension methods. Before that PR we were explicit and would throw. If we now are silent, is there a risk that debugging becomes harder?

Answer was:

It depends on your aim with the iteration of all external types. In my debug scenario it seemed a case of finding external eventhandlers. Our scenario is like this: we have many projects using a piece of code (C) in a library (L) which references a certain version of an external library (EL). Continuous changes of this external library made us decide to extract code C into a seperate Nuget (N). This enables better upgrade management in new projects, but we need to keep library L backwards compatible. Maintaining separate branches would mean a lot of work. On new projects the code C is now referenced from Nuget N, but library L is also used. Nuget N references a newer version of EL. The bootstrap loader now throws the reflectiontypeloadexception because of namespace changes in external library EL.Without the break the project otherwise works fine.

I guess this could happen any time a project references 2 (or more) libraries that have references to different versions of the same third party library. Of course I understand your reasoning to be cautious with core changes, but my educated guess would be that this can't really hurt and would be beneficial for more developing parties.


Stephan 23 Jun 2016, 12:42:47

The situation you describe is one where the whole DLL set in ~/bin is indeed not fully consistent, but for a good reason--at least, one that you want to accept as good. Only, I'm afraid of cases where such situation would be an actual anomaly and prevent the site from working properly (ie, not finding a component, property converter...). Silently ignoring the anomaly scares me a little.

I ''think'' another approach was discussed in the past: throw by default, unless the DLL causing the issue has been explicitely added to some sort of exclusion list in web.config. In which case we would not throw. That way, the situation you create would not prevent the site from running.

Would that make sense?


Roelof Nijholt 23 Jun 2016, 12:59:06

That would make sense, I've already looked at possible low level attributes to prevent the conflicting type from showing up, but didn't find anything. The thing is, in our case the external dll is referenced from our own library (which does contain event handlers), so I can't blacklist our library. Blacklisting the external library wouldn't work because it is exposed through the types of our library. Alternatively placing a sort of exclusion attribute on certain classes would also not work as the types need to be iterated first (already throwing the exception) before being able to check the attribute. It would also be less efficient. Perhaps you mean, catch the exception and then throw, unless the library is white listed. That could work. I could modify my pull request in that direction...

Then again, the corresponding appsetting would probably be abracadabra for a lot of users, and I still don't think ignoring the anomaly will be a big problem. Worst case an error will be thrown further down the line. These errors would also be related to external libraries I think, which probably make it more of a responsibility of the library owners than of Umbraco core.


Stephan 23 Jun 2016, 13:11:24

"''Perhaps you mean, catch the exception and then throw, unless the library is white listed.''"

That's it. If the assembly we're scanning (the one that is passed to GetTypesWithFormattedException) is in a while list then don't throw.

"''Worst case an error will be thrown further down the line.''"

Worse case actually is... no error is ever thrown, yet for example a property value converter is not discovered/registered for some reason. And then it can become difficult to figure out what's actually happening.

Now you have a point about external libs. It means you cannot ship an external lib and auto-whitelist it. Each user would have to do it.


Roelof Nijholt 23 Jun 2016, 13:30:31

I think the conflicting libraries wouldn't be libraries containing event handlers, value converters etc. but references of such libraries (as in our case). But just in case, I would opt to swallow the error and log it. Than it wouldn't be silent. The log files would probably be the first place to look for most developers. Optionally combined with the white list approach.


Roelof Nijholt 24 Jun 2016, 13:58:29

@zpqrtbnk have you made a decision? Sorry to nag but we have a number of projects in the pipeline where we need some kind of solution or workaround. Have a good weekend :)


Shannon Deminick 27 Jun 2016, 11:46:54

Hi,

Any solution made to the core will have to wait until 7.5 at the earliest. Since we've already put out a 7.5 beta I am rather against making a change of this magnitude. I know this seems like a small change but based on past learnings, all changes made at this core level will cause another user some problems. As such, this change probably won't make it to a release until 7.6.

My suggestion if you need this immediately, is to run your website on your own fork with this change that you need and we will review this issue in more detail to learn what repercussions this potentially might have.


Roelof Nijholt 27 Jun 2016, 12:40:01

OK, sorry to hear. We do need some sort of solution in the short run, so I will start taking measures for our upcoming projects than. I still hope the request will be approved later on (7.6), as I think it could benefit not only us but other web development companies as well.


Shannon Deminick 27 Jun 2016, 14:30:17

Yes of course, this will remain scheduled for a future sprint until we have 7.5 out and have time for a full review.

It would be of huge benefit (and save some time) if you could describe the exact steps we could use to replicate your issue, then we can review that particular circumstance and investigate others.


Roelof Nijholt 27 Jun 2016, 15:33:58

The best way I can describe it is copied in the first comment of Stephan. This will probable take quite some effort to reproduce. The error was in our case triggered by a namespace change between 2 versions of a (not publicly available) third party library.

Looking at the goal of the code I would say this is to retrieve external event handlers, converters and such. This would not be impaired by the proposed change (logging and skipping conflicting code). But this is where I might be missing something. Perhaps you can think of other use cases.

One suggestion from above, an option to white list libraries (e.g. in UmbraceSettings.config) for which load exceptions would not be thrown, could even keep the default behavior exactly like now.


Stephan 17 Oct 2017, 12:08:18

Not directly merging https://github.com/umbraco/Umbraco-CMS/pull/1349 as I still want to throw by default, but implemented the whitelist thing in:

PR : https://github.com/umbraco/Umbraco-CMS/pull/2249

Review:

In a template, do

var x = TypeFinder.FindClassesOfType();

This should work ok. Then, drop the attached dll and try again. It should throw an exception, because that dll has a dependency on another dll, which is missing. Then, add the following appSettings values and test that you get the expected result:

=> same exception => same exception => no exception, warning in the log => no exception, warning in the log => no exception, warning in the log


Sebastiaan Janssen 18 Oct 2017, 18:00:39

Tested the above scenarios:

First, on current 7.7.3 => exception (repro of issue)

=> same exception => same exception => no exception, warning in the log {code} 2017-10-18 10:55:14,293 [P12216/D10/T34] WARN Umbraco.Core.TypeFinder - Could not load all types from TestLib.. Exception: System.Reflection.ReflectionTypeLoadException: Could not load all types from "TestLib, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null" due to LoaderExceptions, skipping: . System.IO.FileNotFoundException: Could not load file or assembly 'TestLibDependency, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null' or one of its dependencies. The system cannot find the file specified. {code} => no exception, warning in the log => no exception, warning in the log

Code looks good, functionality is the same as before this change unless web.config is updated so all good.


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted: Pull request

Affected versions:

Due in version: 7.7.4

Sprint: Sprint 70

Story Points: 1

Cycle: