OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.44k stars 2.4k forks source link

Configuring instance of `IShapeTableProvider` does not to the IoC container correctly without `[Feature()]` attribute #14103

Closed MikeAlhayek closed 1 year ago

MikeAlhayek commented 1 year ago

Describe the bug

I have the following startup class

[Feature("CrestApps.Components.User.Avatars")]
public class AvatarStartup : StartupBase
{
    public override void ConfigureServices(IServiceCollection services)
    {
       //....
        services.AddScoped<IShapeTableProvider, AvatarShapeTableProvider>();
    }
}

If I enable the CrestApps.Components.User.Avatars feature, the AvatarShapeTableProvider is registered correctly. If I disable the feature after that, the ShellContext is released. However, the AvatarShapeTableProvider does not get removed from the IoC even when the AvatarStartup does not get called again. It's like it gets cached or registered in a global IoC container instead of the tenant's container which is very strange.

To solve the problem, I had to explicitly add the [Feature("CrestApps.Components.User.Avatars")] to the AvatarShapeTableProvider class.

It is my understanding that [Feature("...")] attribute is required on the controller only.

@jtkech have you seen this issue before? It is very strange to have to add [Feature("...")] for every implementation of IShapeTableProvider. Using ConfigureServices() should be all that we have to do.

jtkech commented 1 year ago

Yes, shape descriptors are cached among all tenants, then the shape table including descriptors and binding strategies is cached per tenant theme, so the strategy injection is only needed when first building the shape table, but somewhere they are tied to features so that if a feature is disabled the related strategy is not concretely executed (if this strategy is tied to this feature).

MikeAlhayek commented 1 year ago

@jtkech Very strange to cache descriptors across all tenants like this. Is there a good reason to do that instead of caching it per tenant... we'll beside the extra memory/cache storage?

But event when we cache them across all tenants, we should only execute the descriptors based on current tenants enabled features not installed features.

jtkech commented 1 year ago

Across all tenants not in the memory cache, for perf so that a shape descriptor is built once even if it is no longer used. In the memory cache of a given tenant we only set the shape table with descriptors and strategies related to a given tenant (and their related features) and per theme.

jtkech commented 1 year ago

Also sharing descriptors decreases a lot memory allocations.

ns8482e commented 1 year ago

You don't need feature attribute on shapetableprovider if there is only one shapetableprovider in a module and there no multiple feature in a module