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

Support for .NET Core 3.x #517

Merged

Conversation

ltines
Copy link
Contributor

@ltines ltines commented Apr 15, 2020

Adds support for .NET Core Dependency Injection interfaces (https://github.com/dotnet/extensions/):

  • custom IServiceProviderFactory to provide Castle Windsor provider as IServiceProvider implementation
  • custom ServiceScopeFactory which begins Castle Windsor scope
  • any services registered in IServiceCollection will be registered with the container

There are different semantics for lifestyles between MS DI and Castle Windsor:

  • MS DI Transient = always new instance resolved but instances are tracked and they are disposed on scope dispose
  • MS DI Singleton = they are linked to root scope since .NET expects specific dispose ordering

Fixes #493

@ltines
Copy link
Contributor Author

ltines commented Apr 15, 2020

I can't seem to be able to build solution becase of errors in other projects. But the 2 new ones should be ok

@ltines ltines changed the title Netcore 3 Support for .NET Core 3.x Apr 15, 2020
@generik0
Copy link
Contributor

@ltines I didn’t know you had anything ready.
All building, all good? Is the build producing a nuget?

@ltines
Copy link
Contributor Author

ltines commented Apr 19, 2020

@generik0 The project builds but I can't make the whole solution to build on mine MSVS due to errors in other projects - i guess its becase of some missing setup. but CI seems to be building fine.

Can you give it a try and build it?

@generik0
Copy link
Contributor

generik0 commented Apr 19, 2020

@ltines can you give me access to your repo. Looks like MS have change .cproj format for iconurl

Very strange though. I build multiple projects with the same property.. "PackageIconUrl". Strange it isn't working here.
Is this the issue you are getting?

image

@generik0
Copy link
Contributor

@jonorossi can you please make a branch the PR can land to? Or do you wnt it directly into master from the PR?

@dquist
Copy link
Contributor

dquist commented Apr 19, 2020

I'm also getting the deprecated iconUrl errors when trying to build on VS2019.

image

Edit: I also get the same errors on master and the build still succeeds in both cases 🤷‍♂️.

@ltines
Copy link
Contributor Author

ltines commented Apr 19, 2020

I don't think the errors are related to my changes

@generik0
Copy link
Contributor

generik0 commented Apr 19, 2020

If i add ;NU5048 to the commpn.props. All is good
image

And the icon is being added anyway,
image

I see recent issues on nuget githib. Maybe ahve MS messed up the nuget packaging a little, again...

image

image

Also new nugets: :-)
image

@ltines and @dquist best to run the build though the build.cmd in the roo for the repo. Is my experience.

@generik0
Copy link
Contributor

@jonorossi ok to add NU5048 to no warn?

@generik0
Copy link
Contributor

@ltines i think you (or someone) needs to add uTest to the sln for the extensions. Maybe look at the aspnetcore facility for inspiration. I have a feeling, no tests, no merge.. (Kust the messenger here)

Copy link
Contributor

@generik0 generik0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ltines a quick review for you :-)

(Update)

@ltinies I see the web review mechanism didn’t always catch my line selection. I will try to fix tomorrow


public Burden GetCachedInstance(ComponentModel model, ScopedInstanceActivationCallback createInstance)
{
if(model.Configuration.Attributes.Get(NetCoreTransientMarker) == Boolean.TrueString ){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line for "{"?

// See the License for the specific language governing permissions and
// limitations under the License.

// The MIT License (MIT)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonorossi Copyright (c) 2016 Volosoft in Castle code? Is it needed since Volosoft is MIT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would like to give them credit since i copied the source as a whole

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ltines you can't do that, the author/copyright-holder of the code is the only person allowed to relicense it, and we like to keep all of Castle under the Apache 2.0 license otherwise it gets complicated. We'd need them to relicense it.

The "copyright Castle Project" part effectively is short for "copyright Castle Project contributors" as no copyright assignment is made. OpenStreetMap a lot of time has the same shorthand for "copyright OpenStreetMap contributors".


public static string OriginalComponentName(string uniqueComponentName)
{
if(uniqueComponentName == null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

private static string UniqueComponentName(Microsoft.Extensions.DependencyInjection.ServiceDescriptor service)
{
return
(service.ImplementationType?.FullName ??
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

current.Value = parent;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tab indentaion?



public void Dispose()
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tab indentaion

{
this.parent = parent;
scopeCache = new ScopeCache();
Nesting = (parent?.Nesting ?? 0) + 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed



public virtual NetCoreRootScope RootScope => rootScope;
public virtual int Nesting {get; private set;}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this "Nesting" used for? i dont see anything but setting for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed. It was used for debugging .NET Core scope nesting

@ltines
Copy link
Contributor Author

ltines commented Apr 19, 2020

@generik0 cheers for the review - hopefully fixed all the coding style issues.

With the UT its a good point. The problem though is - there is a project for UT but it needs to use xUnit because its based on .NET Core specification tests.

What's the preferred UT framework here?

@generik0
Copy link
Contributor

I think it is xUnit in the Castle. Check out the aspnetcore tests :-)

@ltines
Copy link
Contributor Author

ltines commented Apr 22, 2020

@generik0 aspnetcore is using NUnit. Anyway managed to build it and tests are passing

@generik0
Copy link
Contributor

Good to know, and great work dude...

@castelobranco
Copy link

castelobranco commented Apr 23, 2020

Is there a nuget package for that?

@generik0
Copy link
Contributor

generik0 commented Apr 23, 2020

The contributing.md says we should be able to get from a nuget feed:
But the feed is not available :-(
https://github.com/castleproject/Home/blob/master/prerelease-packages.md

image

So until @jonorossi is available, and makes a branch/accepts PR + pre-release, I think you need to grab from build server and provide using local nuget folder:
https://ci.appveyor.com/project/castleproject/windsor/builds/32370836/artifacts

@dquist
Copy link
Contributor

dquist commented Apr 23, 2020

The pre-release feed has been discontinued.

Pre-Release Feed Unavailable: As of 2017-06-11 we no longer use TeamCity for our builds and this server has been decommissioned, therefore the NuGet package feed is no longer available.

Fixes wrong order of initializing options
@ltines
Copy link
Contributor Author

ltines commented May 25, 2020

@robsonj have some sample project? happy to debug
as for the logging - interesting idea but would do it as a separate project - not everyone wants to use Windsor Logging facility as Logging provider for .NET

Ok, reproduced, I was being an idiot (I had .UseWindsorContainerServiceProvider() commented out). It is actually easily reproducible in the sample app. I have attached a zip of that solution.

In Startup.cs, if you put a breakpoint on the var conventions = options.Conventions; line, you'll see that options.Conventions is null.

If you then comment out the line .UseWindsorContainerServiceProvider() in Program.cs and re-run, you'll see that options.Conventions is then correctly, not null.

WindsorNetCore31Tester-master.zip

Sorry, took few weeks but finally tracked down the issue. Can you try latest?

@ltines
Copy link
Contributor Author

ltines commented May 25, 2020

I updated my tester project to verify that lifestyles are maintained.

I am observing that while ScopedToNetCoreScope lifestyle is correctly respected, transient components injected in the controller do not get released when the controller is released. I was not expecting that.

https://github.com/AntonioDrusin/WindsorNetCore31Tester

If it is helpful I can try to write a unit test for that.

Transient components get released when IServiceScope is disposed.

@robsonj
Copy link

robsonj commented May 26, 2020

@robsonj have some sample project? happy to debug
as for the logging - interesting idea but would do it as a separate project - not everyone wants to use Windsor Logging facility as Logging provider for .NET

Ok, reproduced, I was being an idiot (I had .UseWindsorContainerServiceProvider() commented out). It is actually easily reproducible in the sample app. I have attached a zip of that solution.
In Startup.cs, if you put a breakpoint on the var conventions = options.Conventions; line, you'll see that options.Conventions is null.
If you then comment out the line .UseWindsorContainerServiceProvider() in Program.cs and re-run, you'll see that options.Conventions is then correctly, not null.
WindsorNetCore31Tester-master.zip

Sorry, took few weeks but finally tracked down the issue. Can you try latest?

I for sure will today. Thank you for taking a look. The latest version looks to be v0.0.674

@robsonj
Copy link

robsonj commented May 26, 2020

@robsonj have some sample project? happy to debug
as for the logging - interesting idea but would do it as a separate project - not everyone wants to use Windsor Logging facility as Logging provider for .NET

Ok, reproduced, I was being an idiot (I had .UseWindsorContainerServiceProvider() commented out). It is actually easily reproducible in the sample app. I have attached a zip of that solution.
In Startup.cs, if you put a breakpoint on the var conventions = options.Conventions; line, you'll see that options.Conventions is null.
If you then comment out the line .UseWindsorContainerServiceProvider() in Program.cs and re-run, you'll see that options.Conventions is then correctly, not null.
WindsorNetCore31Tester-master.zip

Sorry, took few weeks but finally tracked down the issue. Can you try latest?

I for sure will today. Thank you for taking a look. The latest version looks to be v0.0.674

It's unfortunately still broken. I have attached a screenshot of what I'm referring to, the options.Conventions seems to be null, it is not when using the volosoft integration. I tested against v0.0.674 of Castle.Windsor.Extensions.DependencyInjection.

Screen Shot 2020-05-26 at 10 31 35 AM

@ltines
Copy link
Contributor Author

ltines commented May 31, 2020

@robsonj have some sample project? happy to debug
as for the logging - interesting idea but would do it as a separate project - not everyone wants to use Windsor Logging facility as Logging provider for .NET

Ok, reproduced, I was being an idiot (I had .UseWindsorContainerServiceProvider() commented out). It is actually easily reproducible in the sample app. I have attached a zip of that solution.
In Startup.cs, if you put a breakpoint on the var conventions = options.Conventions; line, you'll see that options.Conventions is null.
If you then comment out the line .UseWindsorContainerServiceProvider() in Program.cs and re-run, you'll see that options.Conventions is then correctly, not null.
WindsorNetCore31Tester-master.zip

Sorry, took few weeks but finally tracked down the issue. Can you try latest?

I for sure will today. Thank you for taking a look. The latest version looks to be v0.0.674

It's unfortunately still broken. I have attached a screenshot of what I'm referring to, the options.Conventions seems to be null, it is not when using the volosoft integration. I tested against v0.0.674 of Castle.Windsor.Extensions.DependencyInjection.

Screen Shot 2020-05-26 at 10 31 35 AM

@robsonj
Yeah I relized that. Or rather my change fixes that single issue but breaks acceptance tests.

So after a lot more digging through aspnetcore and extensions and windsor sources the problem is:

  • when injecting IEnumerable<Service> .NET expectes the instances in the same order they were registered.
  • For RazorPagesOptions there are two related registrations - Microsoft.Extensions.DependencyInjection.RazorPagesOptionsSetup, Microsoft.Extensions.Options.ConfigureNamedOptions[Microsoft.AspNetCore.Mvc.RazorPages.RazorPagesOptions] (in this order)
  • To work around the fact Windsor can't register the same component type for different services, it's registered with IsDefault(). This marks Microsoft.Extensions.Options.ConfigureNamedOptions as a default (same type different instances is used for different options)
  • calling ResolveAll eventually calls INamingSubSystem.GetHandlers. This puts default components / handlers as first thus breaking the "in registration order` requrement
  • because of this wrong order Microsoft.Extensions.Options.ConfigureNamedOptions[Microsoft.AspNetCore.Mvc.RazorPages.RazorPagesOptions] is called before Microsoft.Extensions.DependencyInjection.RazorPagesOptionsSetup

Does anybody has better idea how to register the same class multiple times for different services without default?

@Servox3
Copy link

Servox3 commented Jun 4, 2020

Please forgive me, I didn't read a lot of the thread, so maybe I'm totally off, but would named instances help?

container.Register(
    Component.For<Interface1>().ImplementedBy<Implementation>().Named(Guid.NewGuid().ToString()),
    Component.For<Interface2>().ImplementedBy<Implementation>().Named(Guid.NewGuid().ToString())
    );
var s1 = container.Resolve<Interface1>();
var s2 = container.Resolve<Interface2>();

@generik0
Copy link
Contributor

generik0 commented Jun 5, 2020

@ITUNES, Sorry i did not see this before.
Yes as pointed out by @servox you can name the registrations and you can register multiple.

Is it the MS tests that need the resolving in the correct order, or the actual web/host runtime?

@ltines
Copy link
Contributor Author

ltines commented Jun 6, 2020

@Servox3 , @generik0

it's already using named instances but has other limitations. The problem is to keep the resolved order the same as registered regardless of whether it's default, named or fallback.

Anyway i finally cracked it. @robsonj have a look pls - it resolves Options setup in right order and it passes all spec tests.

Had to override GetHandlers from naming subsystem.

@jonorossi i did some renaming to remove NetCore prefix

@robsonj
Copy link

robsonj commented Jun 9, 2020

@Servox3 , @generik0

it's already using named instances but has other limitations. The problem is to keep the resolved order the same as registered regardless of whether it's default, named or fallback.

Anyway i finally cracked it. @robsonj have a look pls - it resolves Options setup in right order and it passes all spec tests.

Had to override GetHandlers from naming subsystem.

@jonorossi i did some renaming to remove NetCore prefix

Thankyou @ltines , I checked the Options and everything looks great now!

Ready for a release version from my perspective :-)

@generik0
Copy link
Contributor

generik0 commented Jun 9, 2020

@Itines, Will the implementation work with net core 2.x? Or only 3+?

Maybe we should try to get a prerelease out?

@ltines
Copy link
Contributor Author

ltines commented Jun 9, 2020

@Itines, Will the implementation work with net core 2.x? Or only 3+?

Maybe we should try to get a prerelease out?

AFAIK only 3.x since between 2 and 3 they changed the DI.

@generik0
Copy link
Contributor

@jonorossi Any chances of building a pre-release to nuget.org on this branch. It will be easies for many to test....?

@jonorossi
Copy link
Member

@ltines Once you move the docs into the right place, I'll merge this PR and publish a prerelease build for everyone to test out.

@ltines
Copy link
Contributor Author

ltines commented Jun 15, 2020

@ltines Once you move the docs into the right place, I'll merge this PR and publish a prerelease build for everyone to test out.

Done. Thanks @jonorossi for the help.

@jonorossi jonorossi merged commit 7877e3d into castleproject:extensions-dependencyinjection Jun 17, 2020
@jonorossi jonorossi added this to the vNext milestone Jun 17, 2020
@jonorossi
Copy link
Member

Merged and v5.1.0-beta001 published to nuget.org. Thanks.

/// <summary>
/// Naming subsystem based on DefaultNamingSubSystem but GetHandlers returns handlers in registration order
/// </summary>
internal class DependencyInjectionNamingSubsystem : DefaultNamingSubSystem
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding this sub-system to an existing container wipes out all its components. Furthermore, the sub-system looks virtually identical to the base implementation.

Can someone tell me why this is needed for .NET core 3.1? Is there something different about .NET core 3.1 that requires this particular class? I've compiled my own version that doesn't include this sub-system, just to test, and it seems to work the same way regardless.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @robertcoltheart
I tried removing the subsystem when i refactored some of this code. From what I remembered it made all or many if the unit tests fail.

If you look at the new implementation you on master you will see you can override the sub system.

Could you please make a new issue, and reference the current implementation and we can look deeper into it together?

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.

Support for ASP.NET Core 3.0
9 participants