Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not use reflection only if it would fail #821

Merged
merged 1 commit into from
Apr 28, 2015

Conversation

tpetricek
Copy link
Member

I was tying to use F# Data on Azure with Suave using a setup similar to the one in the recent Scot Hanselman's blog. The peculiar thing here is that this is just loading app.fsx using FAKE/F# Compiler Service and running it from the source code (which I quite like because it is super simple).

The problem here is that we are now running the design time component of F# Data on a machine that does not have anything installed and so it was failing to load FSharp.Core.dll version 4.3.0.0. I was running this via FAKE which has correct binding redirects and I had 4.3.1.0 in the folder, but it was still failing.

The code was failing here (1) after we loaded the FSharp.Data.dll assembly using ReflectionOnlyLoad here (2). Now, the problem is that ReflectionOnlyLoad ignores binding redirects - and furthermore, (I think that) once it loads the assembly in (2), it does not give us any chance to fix the situation when it fails to get a type from it in (1).

So, this PR tests checks if we would be able to run the code in (2) using assembly loaded with the reflection only method - if yes, then it does not change anything. If getting a type would fail later, then it falls back to the Assembly.Load approach (which uses binding redirect and worked fine for me).

NOTE: I don't really understand how this works, so I'm not sure what consequences the change has. But I hope the check only applies in situations where the current approach will fail, so I hope it is OK!

@ovatsus
Copy link

ovatsus commented Apr 28, 2015

Looks safe enough, I don't think anyone really understands how this reflection mess works :/

ovatsus pushed a commit that referenced this pull request Apr 28, 2015
Do not use reflection only if it would fail
@ovatsus ovatsus merged commit 14967c3 into fsprojects:master Apr 28, 2015
@tpetricek
Copy link
Member Author

Looks safe enough

That's what I thought. But apparently none of the builds work anymore. Has this been broken before my change, or do we have an amazing test suite that catches subtle reflection load bugs?

@tpetricek
Copy link
Member Author

Running build All on my machine crashes the NUnit test runner:

c:\Tomas\Public\FSharp.Data\packages\NUnit.Runners\tools\nunit-console.exe "-nologo" "-noshadow" "-labels" "c:\Tomas\Public\FSharp.Data\tests\FSharp.Data.Tests\bin\Release\FSharp.Data.Tests.dll" "-xml:TestResults.xml" "-framework:4.5" "-domain:Multiple"

Unhandled Exception: System.IO.FileLoadException: Could not load file or assembly 'nunit-console-runner, Version=2.6.4.14350, Culture=neutral, PublicKeyToken=96d09a1eb7f44a77' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)
File name: 'nunit-console-runner, Version=2.6.4.14350, Culture=neutral, PublicKeyToken=96d09a1eb7f44a77'
at NUnit.ConsoleRunner.Class1.Main(String[] args)

@ovatsus
Copy link

ovatsus commented Apr 29, 2015

Humm, I'll have to have a better look

@veikkoeeva
Copy link

@tpetricek, @ovatsus This sounds exactly like the problem there was in another project. The solution might be as simpe as using Appdomain.Applypolicy. More back-and-forth with reasoning and more links to relevat subject matter can be read from here.

@tpetricek
Copy link
Member Author

@veikkoeeva This is cool, I didn't know this existed. I'll see if that fixes the issue instead of my ugly hack :-).

@veikkoeeva
Copy link

@tpetricek You are welcome. I'd like to take the chance and point out there is some variation in the rules depending on the platform and now that CoreCLR is coming. From the aforementioned link, some of interest could be:
Best Practices for Assembly Loading
Redirecting Assembly Versions and you may need to resort to ResolveEventHandler (example here) to apply the binding rules. Perhaps one difficult part is to choose of all the functions those that resolve and load also the dependencies with redirects.

tpetricek added a commit to tpetricek/FSharp.Data that referenced this pull request May 1, 2015
ovatsus pushed a commit that referenced this pull request May 1, 2015
Use ApplyPolicy with ReflectionOnlyLoad based on name (improve #821)
@weismat
Copy link
Contributor

weismat commented May 5, 2015

@veikkoeeva
Copy link

@weismat Debugging would tell for sure. The error message would the same if there were a failure to apply a policy (binding redirect) and factually the right assembly would in the probing path. I'd be inclined to answer: yes.

@tpetricek
Copy link
Member Author

@weismat It sounds like the same issue, but I'm a bit puzzled why this would be happening - because you would not typically have/load the FSharp.Data.DesignTime.dll on the production server (I do it because I run the code via FAKE...). If you can test with new version of F# Data (and perhaps share your Fusion log), that would be amazing!

@weismat
Copy link
Contributor

weismat commented May 5, 2015

I am typical just moving the bin/xxx dir content from my dev to my prod machine.
I have now changed the FSharp Core version back to the original F# 3.1, upgraded the FSharp Data nuget package in the f# projects and it is working. The project is a mixture of C# and F#. The main console app is written in C# and the F# libs do the data retrieval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants