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

Split to actors and services projects #60

Open WhitWaldo opened 2 years ago

WhitWaldo commented 2 years ago

Implemented the ask at https://github.com/autofac/Autofac.ServiceFabric/issues/29 and split out to two projects: Autofac.ServiceFabric.Services (stateless and stateful use the same packages) and Autofac.ServiceFabric.Actors.

I further split out the tests to apply to each separately and corrected the various failures that popped up. I experienced the same issue as reported at https://github.com/autofac/Autofac.ServiceFabric/issues/49 and corrected it simply by eliminating use of strongly-named assemblies throughout (if you mark any of the projects in the solution as signed, re-run the tests and you'll see about 3/4 of them fail with "Access denied". Changing the InternalsVisibleTo attributes from [assembly: InternalsVisibleTo(InternalsVisible.ToDynamicProxyGenAssembly2)] to [assembly: InternalsVisibleTo("DynamicProxyGenAssembly2")] cleared up all those issues (and the only difference is using a non-signed reference from a non-signed assembly.

The .NET documentation about strong-naming read that "For .NET Core and .NET 5+, strong-named assemblies do not provide material benefits. The runtime never validates the strong-name signature, nor does it use the strong-name for assembly binding.", so I would propose dropping it altogether (as I experienced the same issue myself, prompting today's contribution).

If you must have a signed version, I'd suggest creating yet additional packages on top of these (might I suggest Autofac.ServiceFabric.Actors.Signed and Autofac.ServiceFabric.Services.Signed) that themselves are explicitly signed for whatever purpose you need them for? If you do this, note that you'll need to update the various assemblies using the strong-name version two paragraphs up.

I didn't make any changes to CI/CD to enable the building and deployment of the separate packages as I'm not familiar with Appveyor.

WhitWaldo commented 2 years ago

Note on this PR - once accepted, I'd be happy to update the examples project as well to reflect the latest and greatest out there to close out https://github.com/autofac/Autofac.ServiceFabric/issues/26

tillig commented 2 years ago

Since we're targeting netstandard and not a specific .NET Core version, we do need to keep the strong naming. We don't have any precedent of having specifically signed packages; if we were to do that it would likely go the other way, having .Unsigned. But I'm not super interested in owning or supporting multiple package versions.

What is the value of splitting this into two packages? This is really amazingly breaking - you can't just take updates anymore, you have to "know" that the packages changed.

WhitWaldo commented 2 years ago

@tillig The main value is that consumers no need to add the Microsoft.ServiceFabric.Actors (and potentially Microsoft.ServiceFabric.Services.Remoting) dependencies to SF services addressing the ask at https://github.com/autofac/Autofac.ServiceFabric/issues/29, which also lessens the annoyance of dealing with https://github.com/autofac/Autofac.ServiceFabric/issues/18.

As a secondary matter, I experienced the same issue reported here both when trying to consume the Autofac.ServiceFabric package off NuGet in my .NET 6 project and after splitting these projects in the unit testing.

I ultimately fixed the "Access is denied" issue in the unit tests by simply changing the attribute [assembly:InternalsVisibleTo(InternalsVisible.ToDynamicProxyGenAssembly2)] to [assembly:InternalsVisibleTo("DynamicProxyGenAssembly2")]. The former sets the public key in the string and the latter doesn't. With that simple change, the (unsigned) unit tests suddenly worked fine. I've built this version and deployed on my own NuGet distribution to consume in my own various unsigned projects and I have yet to experience this "Access is denied" issue again.

I haven't found anything definitive that speaks to why including the strong name for a package reference in an attribute should matter in this manner, but in my own testing it has, so that's second reason for this approach - it fixes https://github.com/autofac/Autofac.ServiceFabric/issues/49 (though again, not clear why/how).

That said, makes sense to go with .Unsigned instead of .Signed, sure. If the only differences are which attribute is used and the rest handled by the build pipeline, is the cost of owning them both side-by-side that high?

tillig commented 2 years ago

I'm not sure having extra dependencies that go unused is necessarily something... that, honestly, I care a whole lot about. I admit I've not personally been involved in this particular library since its inception, leaving it to @alexmg, but he's largely stepped back from Autofac for the moment. I will probably close #29 because, just like creating an ASP.NET MVC project, you may get some dependencies you won't use, but simply having them there isn't going to hurt anything. Just don't use them.

I do see #18 being an issue, but reducing the warnings from three to two doesn't really address it well.

Short version - I don't think we really want to take on splitting the packages. At least, I, personally, don't. If you'd like to come on as a permanent owner for this library, I'm open to that. In that case, it'd be up to you to monitor StackOverflow and issues for the questions, update the docs and the examples, write up the release notes, and generally own and support this from here on out. We absolutely need more people to help and it'd be awesome if you want to own it.

However, it looks like there are about 130K downloads of the current version so it's not necessarily just five or six people that'd get hit by this. It will require some docs, support, and handholding. Let me know if you're up for that.

It would be interesting to pull on the thread to find out why #49 gets fixed like this and see if there are other solutions before just dropping library signing. Again, I recognize it's not interesting as much in .NET Core, but we're not targeting only .NET Core here. If we can understand exactly why this fixes it, I'd be much more inclined to accept that as a breaking change. Going signed-to-unsigned or changing a signature is a real problem. I remember log4net did this between 1.2.10 and 1.2.11 and it was a huge challenge trying to rectify libraries that referenced one with libraries that referenced the other. It falls down hard.

WhitWaldo commented 2 years ago

Speaking to your response on #29 too, Service Fabric packages aren't updated independently, so the split packages here wouldn't result in one package being newer than the other like in other projects.