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

Bugfix ms di no available scope #662

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

rvdginste
Copy link
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

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
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
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

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?

@lmorvan
Copy link

lmorvan commented Jan 12, 2024

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