search

autofac / Autofac.ServiceFabric

Autofac integration for Azure Service Fabric. Provides service factory implementations for Actors, Stateful Services and Stateless Services.
MIT License
26 stars 27 forks source link

Upgraded Autofac to 5.1 #53

Closed pcherpitel closed 4 years ago

pcherpitel commented 4 years ago

Upgraded Service Fabric packages to 7.0.466 Dropped support for .Net 4.5 Upgraded package template (icon and license)

pcherpitel commented 4 years ago

@alexmg @tillig the changes are pretty straight forward since it's mostly nuget packages updates let me know what you think about it and if you would like some changes before I publish the PR.

alexmg commented 4 years ago

Looks good @pcherpitel. A few small tweaks and it's good to go!

pcherpitel commented 4 years ago

@alexmg @tillig Can you link the PR to https://github.com/autofac/Autofac.ServiceFabric/issues/52 ?

alsami commented 4 years ago

Just as an info we need to finish this PR ahead of this since Autofac.ServiceFabric relies on Autofac.Extras.DynamicProxies.

pcherpitel commented 4 years ago

Just as an info we need to finish this PR ahead of this since Autofac.ServiceFabric relies on Autofac.Extras.DynamicProxies.

From what I understand the issue in Autofac.Extras.DynamicProxies affect interface interceptors which are not used in Autofac.ServiceFabric. In this context since the changes in Autofac 5.0 are causing runtime failures if we use Autofac.ServiceFabric 2.2 can we move forward with this PR? @tillig @alexmg @alsami

tillig commented 4 years ago

That's a good point, I don't see DynamicProxy as being referenced here. @alsami can you clarify before we move forward here?

alsami commented 4 years ago

I am not a hundred percent sure, if going forward without the fix of DynamicProxy is safe. I would wait for the fix to be merged and then update this PR accordingly. Maybe users end up using that functionality in their project anyway because it's a transitive dependency. If it's save to go on without the fix then consider my comment obsolet :)

tillig commented 4 years ago

Oh, oh, oh, I see it now. There is a reference to Autofac.Extras.DynamicProxy. The diff had it hidden from me. I agree, we need to finish that one first so we can update the reference here.

pcherpitel commented 4 years ago

Autofac.ServiceFabric is using Autofac.Extras.DynamicProxies but only for class interceptors which are not relying on ActivatingHandlers so they are not impacted by the changes of Autofac 5.0. Autofac.Extras.DynamicProxies interface interceptors are definitely impacted by the changes of Autofac 5.0 because they rely on ActivatingHandlers and since this PR https://github.com/autofac/Autofac/pull/1043 was merged back ActivatingHandlers can be called twice. The current situation is just a bit frustrating because nothing prevent users of Autofac.ServiceFabric to update to Autofac 5.0 then they end up with runtime failure. Letting the PR go through will at least limit the impact to users that are using Autofac.Extras.DynamicProxies with interface interceptors.

alsami commented 4 years ago

The current situation is just a bit frustrating because nothing prevent users of Autofac.ServiceFabric to update to Autofac 5.0 then they end up with runtime failure.

We very much understand that this is frustrating and causes users to have problems. Which is why we need to make sure that we don't create more possible problems. We are working on the fix and as soon as that is done, we will get your PR ready and merge it :+1:

alexmg commented 4 years ago

The other PR has been unblocked now.