-
Notifications
You must be signed in to change notification settings - Fork 456
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
NET 6.0 ForcedScope Failed with "No Available Scope" on GET request #646
Comments
Looking at a diff (v5.1.2...v6.0.0), the |
Same behavior: moving from 5.1.1 to 6.0, all tests are broken... |
To be clear, as fair as I'm aware no one is looking at fixing this, someone will need to step up to fix it. Make sure to add unit tests so it doesn't get broken again. |
Do you mean that it's not considered as a bug? Weird... |
No, I mean this is an open source project that needs contributors. |
Ok, understood. We have reverted our code to 5.1.2 but as soon as I have a time slot, I will do my best to help on this one. |
I was involved with the original change for #577 (initial commit was a minimal change in Extensions.DI and was related to the root scope, not clear what changes were done afterwards) and will look at. If someone can add a unit test that exposes the bug, that would be super useful! |
Hi, I already had a look at it this weekend using the original commits on the branch (commits were squashed before merge) and from my tests it seems the initial commits were fine and the bug was introduced in later commits (as part of the same change). #577 was a fix for #563 and I'd really like to implement a fix instead of reverting everything. My next steps are to first add an extra unit test. I could reproduce the error when actually setting up and running a minimal Asp.Net Core app (as reported by @fsalas ), but not without Asp.Net Core. I will add a test that uses the Asp.Net Core testing facility and see if that works for the test case. Then I will work on the fix itself and use the new test as a reference. Is there a specific reason you need to upgrade to 6.0, or can you hold off for now and stick with 5.1.1/5.1.2? I plan to have a fix for this by the end of the coming weekend. I use this extension myself on several applications that are in active development, so it's in my own interest to have it fixed sooner rather than later. |
The "No Scope Available" exception might be caused by threads spawn from the Looking at the code I think the problem lies in ExtensionContainerScopeCache which uses an AsyncLocal to track the current scope. It's pretty straightforward setup an example that shows that AsyncLocal are uninitialized in threads spawn that way.
previously this it was "working" because there was no null check for the Current Scope cache, null was an allowed value. |
…removed null check on scope cache (AsyncLocal can be null on Threads coming from Threadpool.UnsafeQueueUserWorkItem, having no null check was also the original behavior)
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.
…removed null check on scope cache (AsyncLocal can be null on Threads coming from Threadpool.UnsafeQueueUserWorkItem, having no null check was also the original behavior)
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.
I created a bugfix for this in PR #662. The scope implementation is now again closer to what @ltines implemented originally, but with the essentials of the earlier change preserved. I added a test to simulate what @fsalas reported using Not sure whether this will work in all circumstances and did not have time yet to test this in a real application, but the implementation is a lot closer to what it was originally (implementation of @ltines). |
Great work @rvdginste. Can I get some people on this ticket to at minimum code review the changes, would be great if someone can test them in their own application. |
I tested the fix in "production code" and it does not work, the issue moved to Root Scope Accessor. In your latest commit @rvdginste Root scope is cached in an AsyncLocal (originally it was a plain static property), now I get "no root scope" exceptions here:
my solution combines AspNetCor and Akka.net and does lots of resolution on background and threadpool threads. the root the problem is still the same: AsyncLocal is uninitialized in thread created in an unsafe way. Another way to always make things that use AsyncLocal fail is call: ExecutionContext.SuppressFlow() and try to resolve something. I'll try to setup another test that reproduce the issue on root scope accessor too. |
Here are a couple more cases that can fail:
Trying to resolve (with the underlying Castle Windsor) something that has a lifestyle that kicks in any scope accessor will result in the usual AsyncLocal problem. It's a pretty ugly case but in my application it happens because of different "adapters" for dependency injection that use the same Castle Windsor container (AspNetCore and Akka, to be more specific, but may happens in many other scenario). Trying to create (or resolve) a Service Provider in an Unsafe Thread has the same problem. EDIT: the typical problematic scenario is like this:
|
Hello again, I had the time to work on the subject again. Take a look at this branch here: I added some more test about resolving services with different lifestyle with WindsorContainer and the adapted IServiceProvider: they show supported and unsupported scenarios (at this stage I let the tests fail on purpose instead of checking for the generated exception, will add that later). A couple of tests (Can_Resolve_LifestyleNetStatic_From_WindsorContainer, Can_Resolve_LifestyleTransient_From_WindsorContainer) deserve attention: they succeed because Transient and Scoped object ends up being tied to the root scope, which is a memory leak, maybe we can find a way to let them fail if we try to resolve those services from the rootscope when using an IServiceProvider. this is however a standard behavior of all containers. I moved the null check from the cache to the Scope Accessors and improved the error message. As an experiment I added a global option to map NetStatic lifestyle to the standard Castle Windsor Singleton lifestyle, I believe that singletons should always be resolved whether we have a root scope or not (this can solve some issues like the one reported in: #639) . EDIT:
|
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.
Hi, I am experiencing a similar issue. Is there any chance this PR could be merged anytime soon, to see if it fixes the problem? @AGiorgetti #664 Thanks in advance |
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. |
I have reproduced the bug with ASP.NET 6 for both gRPC and simple Web API. |
Hey all, thanks for all the work you do. 👍 Been following this issue for a bit; I am experiencing the exact same issue when using the current v6 version. Could the proposed fixes please be made available? Would like to stay on v6+ and not have to revert to v5 if at all possible... |
This Pull Request seems to fix the issue #666 |
Hi everyone, We've got 3 pull requests that I think are meant to solve the same problem. Can pull request authors and others please work together to determine which pull request I'm looking at. Thanks. |
I agree. To be honest, i did not look at the other implementations. I tested this one because of the comment in the PR and it just worked. So i suggest @jonorossi can stick primarily with #666 and if it is okay, merge it. |
@jonorossi. It would be great if this fix could be made available through a NuGet package somehow. |
…removed null check on scope cache (AsyncLocal can be null on Threads coming from Threadpool.UnsafeQueueUserWorkItem, having no null check was also the original behavior)
As a follow up from my comment on the 24th January trying to determine which pull request I should review. I received no replies from pull request authors putting forward support for their own pull requests, so nothing happened. There was a change 2 days ago, #668 was opened superseding #664. We've still got 3 pull requests:
We've got 5 supporters for #666, so if no further conversation happens here by next weekend I'll merging #666 and closing the rest. |
Actually you should check #668 because the null scope is not the only problem of the extension. Due to introduction of keyed service in .NET 8 if you use libraries like Microsoft Orleans (or any other in the future) that uses this new feature for registration, the library would crash. Also in that pull request the null scope is fixed. The PR is quite bit (I need to change lots of thing) and actually is the version I'm start using on our software. Feel free to ask for clarification etc, if you need further information. |
I'll wote for #668. We should definitely support keyed service in .NET 8. |
Support for keyed service in .NET 8 is certainly a good thing to do, yet this issue is about a bug with ASP.NET 6. Is it not too early to push for keyed services? |
These are two unrelated problem. Problem in this issue affects .NET 6 or greater, but if you want to support .NET 8 you surely need to support Keyed service, or you are stuck using .NET up to version 7. My comment was for @jonorossi that previously states
My object was on "closing the rest" because PR #666 only address this issue, but still you will miss support for .NET 8 that was released months ago. So I think that PR #666 can be closed quickly to address this problem, but if you close also #668 you will miss support for .NET 8 |
If #668 also fixes the ASP.Net Core 6+ bug and adds support for keyed registrations, I'm fine with it. It's mid-march and the migration of our solutions to .NET 8 is still blocked due to this issue. Last comment is three weeks ago. Let me ask the other way around: is anyone against #668? If not, @jonorossi please review this one... |
Hi all, had a look through the 3 PRs @jonorossi mentioned above few things:
I have a fix #670 that uses Root scope when the current one is |
Any news? It's July now. Maybe just focus on fixing this bug first (6.0.1 ?) and then adding support for .NET 8 in a separate release. Fixing this bug would at least make 6.x usable in .NET 8 as long as you don't use keyed services, right? I had to downgrade to 5.x to make stuff work on my end for now. |
@nallejacobsson It is not true, if you use in .NET 8 any external library can use keyed service and fails miserably. As an example, Microsoft Orleans is unusable. So, if you want to move to .NET 8, if you do not have keyed service available, you are in a dangerous position, because everything seems to work, until you upgrade one of the library and it starts using keyed service.... |
@alkampfergit Ah, yes, I understand. In that case I vote for fixing both issues at the same time. I wish I was skilled enough to help. I already moved some projects to .NET 8 using 5.x. I guess I'm living dangerously now... Do you know what would happen if an external library tried to use a keyed service? What kind of exception? |
Tbh: I gave up on this. @nallejacobsson Imagine they split the bug fix and the addition of NET 8 support. With the current speed it has been reviewed, we would have the NET 8 support in 2025/2026 i guess? We forked the fix of @alkampfergit and it's working fine for weeks now. Imho, let us focus on what's working and just propose #668. So maybe it is also a solution for you? Every distraction leads to further delays... |
@kbl-schleupen I agree, #668 it is! @jonorossi Do you have time to look it over? |
First of all, great work all of you. I'm trying to port some project of mine to NET 6.0 and AspNetCore, which are new to me.
I've downloaded master, and using the Castle.Windsor.Extensions.Dependency.Injection.
When I run a simple Hello World I'm facing a "No Scope Available exception".
My initialization code is pretty simple, just trying to put all together:
IAplicationInitializer implementation does all the windsor registration.
UseTaxologicCoreInitialization is just
If I run this code, it fails at app.Run() when it gets it first request to / with an exception "No Scope Available" at line 27 of ExtensionContainerScopeCache ,
that comes fron ForcedScope constructor.
For what I can infere from the code, as I'm no expert in Castle internals, the ForcedScope stashes the current Scope, set as current the passed one and restores it on ForcedSope disposal. The problem is that when there is no current scope in the cache it fails when tries to get it to set previousScope in
I did a couple of small changes to prevent that, and the exception has gone.
These are the changes so you can evaluate them and include if you think that has value:
in ExtensionContainerScopeCache I added a getter to know if Current has value
in ForcedScope I changed the Constructor with this
It just prevent the exception if there is no current scope
Looking forward to hear your comments about the changes, or any comments on how to prevent the exception by other means
Thanks in advance
Fernando
The text was updated successfully, but these errors were encountered: