Skip to content

Bugfix ms di no available scope#662

Open
rvdginste wants to merge 9 commits intocastleproject:masterfrom
rvdginste:bugfix-ms-di-no-available-scope
Open

Bugfix ms di no available scope#662
rvdginste wants to merge 9 commits intocastleproject:masterfrom
rvdginste:bugfix-ms-di-no-available-scope

Conversation

@rvdginste
Copy link
Copy Markdown
Contributor

Fixes #646

rvdginste and others added 9 commits August 7, 2023 00:09
Conflict that StackTrace matches a namespace.
To fix issue castleproject#563 (support concurrently existing containers) a fix was
created in castleproject#577.  After the initial fix some refactorings were done on
the scope implementation that were not correct.

This commit changes the scope implementation back to the original
implementation of @ltines, but with the support for concurrent
containers.
@AGiorgetti
Copy link
Copy Markdown

AGiorgetti commented Aug 10, 2023

I tested this code and there are cases in which it still fail in an AspNet Core project.

The main issue is:

  • register a singleton service in the service collection (like a logger).
  • start an Unsafe threadpoool thread
  • resolve the service using the underlying WindsorContainer (other libraries might have their own wrapper around castle) leads to a null reference exception in the lifestyle scope accessors.

I openend another PR (#664) with some more tests and code changes to be evaluated that can help finding a solution

@rvdginste
Copy link
Copy Markdown
Contributor Author

@AGiorgetti @jonorossi

I am starting to think that the safest thing to do now (at least for now) is to revert the code to the original static root sope, so that the code is 100% compatible with the earlier version of the extension. Then we no longer support concurrent containers, but existing code should not have problems and it would allow existing projects to upgrade to the new version.

I do want to explore this further, but I am afraid it will take some time to have a good solution.

What do you think?

Ruben

@jonorossi
Copy link
Copy Markdown
Member

@rvdginste I'm happy to leave that decision to you guys who are using it.

My only request is to keep all the additional unit tests you are writing, and if you plan on restoring concurrent containers in the future keep them separate and mark [Ignore("...")].

/cc @AGiorgetti

@AGiorgetti
Copy link
Copy Markdown

AGiorgetti commented Aug 10, 2023

I have no problem reverting the code as long as we keep all the tests (I added even more checking what happens when you attempt to resolve services with different lifecycles in another PR #664)

Looking at the code of dotnet DI you can see that they use a root scope tied to each ServiceProvider they create from the factory; some of the thing we should investigate to support "concurrent" containers (they are not anyway, because the underlying instance of castle windsor is always the same) - imo - are:

  • create a "root scope" for each IServiceProvider resolved from the factory: allow for multiple rootscope and create them every time the call CreateServiceProvider() from the provider factory, this should be more similar to what dotnet does.
    We should also check the Dispose() method of WindsorScopedServiceProvider: it should not dispose the container.
  • experiment with child containers to simulate dotnet scopes.

Another thing to investigate is how Singleton are handled, I'm not sure having a custom NetStatic lifecycle is a good idea given the fact that under the hood we have a single WindsorContainer instance... why not use the standard Singleton behavior?

@anslmorvan
Copy link
Copy Markdown

For some reason I'm not aware of, my code that used to work with previous versions does not work with the latest. I have added a comment in #595 but I think it's worth having it here.

weixiang-li added a commit to weixiang-li/Windsor that referenced this pull request Jan 17, 2024
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.

NET 6.0 ForcedScope Failed with "No Available Scope" on GET request

4 participants