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

ILRepack causes issues with the PlatformEnlightenmentProvider #264

Closed
dennisdoomen opened this issue Aug 18, 2016 · 15 comments
Closed

ILRepack causes issues with the PlatformEnlightenmentProvider #264

dennisdoomen opened this issue Aug 18, 2016 · 15 comments

Comments

@dennisdoomen
Copy link

When Rx.NET v3 is used in a library as an internal dependency and which uses ilrepack to internalize that dependency before building a Nuget package, and that package is used in another project (which by itself might use a flavor of Rx.NET), the resolution of the platform-specific enlightment provider fails with:

System.InvalidCastException: Unable to cast object of type 'System.Reactive.PlatformServices.CurrentPlatformEnlightenmentProvider' to type 'System.Reactive.PlatformServices.IPlatformEnlightenmentProvider'

With v2, the exact same thing happens, but you could work around that by explicitly setting the enlightment provider like this:

PlatformEnlightenmentProvider.Current = new CurrentPlatformEnlightenmentProvider();
EnlightenmentProvider.EnsureLoaded();

Unfortunately the Current property no longer has a setter. Now what?

@clairernovotny
Copy link
Member

It seem like using ILRepack is a bad idea as Rx is heavily platform optimized. By choosing ILRepack, you're not necessarily getting the correct runtime versions for the right platform. When you use Rx as a NuGet dependency, NuGet ensure's that all of the correct bits line up for the target platform.

What is the scenario where you need to use ILRepack instead of using a direct dependency?

@dennisdoomen
Copy link
Author

I'm trying to prevent my library consumers to end up in dependency hell. In other words, I don't want to bother them with the fact that I'm using Rx.NET internally.

@clairernovotny
Copy link
Member

clairernovotny commented Aug 19, 2016

I appreciate what you're trying to do, but I think that project.json and transitive deps go a long way towards fixing the issue. Part of the issue that you'll hit is that not all libraries are "simple." In the case of Rx, like many other libraries, what your portable library is referencing is not necessarily what you're getting at runtime. That is the case with Rx where the runtime versions are different binaries based on your runtime.

How would you handle a "bait and switch" or any other package that uses reference assemblies?

@dennisdoomen
Copy link
Author

Yeah, you're probably right. And for the part of Rx that I'm using, it's probably not worth using it at all. Since my library is 4.5.2 only, I kind of assumed it would be safe to merge it. We did the same for other corporate components that were .NET 4.5 only and the merging worked just fine.

@gluck
Copy link

gluck commented Aug 19, 2016

We also heavily use this scenario (repacking Rx), benefits is that we have (for instance) modules using Rx 1.11.1111 (yeah I know) running side-by-side (same AppDomain) with others using Rx 2.2.5, and potentially (or not, given this issue 😄 ) with others running Rx3.
It allows for module-by-module non-mandatory upgrades/migrations, and not platform/big-bang style.

Regarding bait and switch, it can be troublesome but feasible:
you need make your package also a bait-and-switch-like (create one assembly per platform by repacking your target assembly with the right dependencies for this platform, and repack the PCL one with the PCL bait assembly).

@gluck
Copy link

gluck commented Aug 19, 2016

Note that ILRepack aside, we also heavily tune the PlatformEnlightenmentProvider, which we will no longer be able to:

  • to name Rx threads appropriately and help troubleshoot thread consumption/leaks
  • to redirect Scheduler.ThreadPool to TaskPool (because exceptions in the former fail fast - i.e. kill the app)
  • to log any Rx exception (IExceptionServices)
  • to redirect the IConcurrencyAbstractionLayer long running tasks to TaskPool instead of ThreadPool (same as above)
  • to workaround this issue

@clairernovotny
Copy link
Member

clairernovotny commented Aug 19, 2016

@gluck It would seem to be worthwhile to address the issues that lead you to tune the PlatformEnlightenmentProvider directly as the goal for 4.0 is to remove enlightenments all-together....the only reason it was even present was to facilitate a weird PCL split without using either cross-compiling or "bait and switch."

One of the goals for v4 is to combine the majority of the code into a single library #199. With that, there'd no longer be a need for that mechanism either.

Can you please file issues for each of the gaps you've outlined?

@gluck
Copy link

gluck commented Aug 19, 2016

Would it mean that the repack story will work again in 4.0 ?
Regarding my reasons, they aren't issues per-say (but the last one, but I haven't reproduced it myself), although I can log requests like:

  • prevent fail-fast from happening when exceptions are raised-and-not-catched in streams or observers (aka remove any use of the ThreadPool or QueueUserWorkItem and prefer TaskPool which doesn't fail-fast since .Net 4.5), making sure they all get raised to UnobservedTaskException (or similar) for logging
  • allow injecting custom thread factory for any thread created by Rx (NewThreadScheduler, EvenLoopScheduler, ThreadPoolScheduler.ScheduleLongRunning, ConcurrencyAbstractionLayer ...)

Think that would be useful ?

@clairernovotny
Copy link
Member

clairernovotny commented Aug 19, 2016

Would it mean that the repack story will work again in 4.0 ?

Maybe? You'd still have to deal with the fact we cross compile for 4-5 platforms, which means that would force you to do the same. I.e.e, you'd need to match our target outputs and nuget package to make sure your users get the correct rx impl.

I strongly discourage repacking Rx.

@clairernovotny
Copy link
Member

Please do log those requests...they can lead to a design discussion around each of them.

For example, is the real issue for fail-fast around logging or around logging? There may be ways to address it without dictating which threading mechanism to use -- like adding a try/catch/finally around some of the inner mechanisms to expose a central exception handling filter. Not sure if that would address the actual need here.

Similarly, I could see a discussion around injecting thread factories (applying a policy of sorts) for an application.

@gluck
Copy link

gluck commented Aug 19, 2016

Here you go #265 / #266

@dennisdoomen
Copy link
Author

dennisdoomen commented Aug 20, 2016

One of the goals for v4 is to combine the majority of the code into a single library #199. With that, there'd no longer be a need for that mechanism either.

This would solve my original problem.

@ButchersBoy
Copy link

Hi guys,

So what about the blog post mentioned by @gluck ? https://dragablz.net/2015/06/09/dealing-with-rx-exception-failed-to-start-monitoring-system-clock-changes/

I am the original author of the original post. The bottom line is PeriodicTimerSystemClockMonitor will occasionally throw an error on "classic" .Net Framework. We have seen this several times in production environments.

I don't wan't to have to be setting a custom enlightenment provider either but we can't trust the current implementation of PeriodicTimerSystemClockMonitor.

So we can't upgrade to v3 at this current time.

Should I raise a separate issue?

@clairernovotny
Copy link
Member

@ButchersBoy please file a separate issue on that so we can track/fix it

@clairernovotny
Copy link
Member

Closing this out as #270 should capture the mentioned issue.

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

No branches or pull requests

4 participants