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

Update context pooling multitenancy sample to more closely approximate a real application #3787

Closed
amiru3f opened this issue Mar 29, 2022 · 13 comments · Fixed by #3822
Closed
Assignees
Milestone

Comments

@amiru3f
Copy link

amiru3f commented Mar 29, 2022

Hi,
As you know the DbContext should be injected as Scoped mode. But I guess the WeatherForecastScopedFactory can be injected as singleton, because there is no need to have a scoped factory object per request (per scope). The singleton WeatherForecastScopedFactory can create a scoped DbContext per scope and the corresponding DbContext will be pooled.
Could you please describe that why the "WeatherForecastScopedFactory" service is injected as scoped?

There is the line which scoped injection is occured:

builder.Services.AddScoped<WeatherForecastScopedFactory>();

@roji
Copy link
Member

roji commented Mar 30, 2022

@amiru3f you probably want to give this doc section a read. To summarize, that sample first registers a singleton pooling DbContextFactory (as is standard), but because of multi-tenancy, a scoped IDbContextFactory is registered as well, which gets injects tenant-specific (and therefore scoped) data into the pooled contexts before handing them over to the user.

@amiru3f
Copy link
Author

amiru3f commented Mar 31, 2022

I read this doc Dear roji, But by the corresponding issue I mean that it can be implemented like the following code and injected as Singleton to reduce memory allocation:

public class WeatherForecastScopedFactory : IDbContextFactory
{
    private readonly IDbContextFactory _pooledFactory;
    private readonly int _tenantId;

    public WeatherForecastScopedFactory(
        IDbContextFactory<WeatherForecastContext> pooledFactory,
        IHttpContextAccessor httpContextAccessor)
    {
        _pooledFactory = pooledFactory;
    }

    public WeatherForecastContext CreateDbContext()
    {
        var context = _pooledFactory.CreateDbContext();
        context.TenantId = GetTenantId();
        return context;
    }

    private int GetTenantId() 
    {
        // In this sample, we simply accept the tenant ID as a request query, which means that a client can impersonate any tenant.
        // In a real application, the tenant ID would be set based on secure authentication data.
        var tenantIdString = httpContextAccessor.HttpContext.Request.Query["TenantId"];
        if (tenantIdString != StringValues.Empty && int.TryParse(tenantIdString, out var tenantId))
        {
            return tenantId;
        }

        return -1; //or throw exception to be caught in another middleware.
    }
}

@roji
Copy link
Member

roji commented Mar 31, 2022

How would that work, given that the tenant ID is a scoped piece of data? In the sample above, HttpContextAccessor is a scoped service - since each HTTP request has a different scope, with its own tenant ID - and so cannot be injected into a singleton service...

@amiru3f
Copy link
Author

amiru3f commented Mar 31, 2022

Dear roji, Thanks for your reply

HttpContextAccessor is a singleton service, not scoped. See this

We can access scoped httpContext using Singleton HttpContextAccessor. And it would work.

@ajcvickers
Copy link
Member

@davidfowl Is using HttpContextAccessor to get the current request preferred over using a scoped service?

@davidfowl
Copy link
Member

Assuming this was a singleton service yes. We don't have anything specific to access the http context with a scoped lifetime

@roji
Copy link
Member

roji commented Apr 9, 2022

@davidfowl the goal here is simply to get an HTTP header from the current request (containing a tenant ID) from a controller - what's the recommended way to do that? I would have assumed some sort of scoped service would provide access to that (because tied to the current HTTP request)

@ErikEJ
Copy link
Contributor

ErikEJ commented Apr 9, 2022

HttpContextAccessor ?

@davidfowl
Copy link
Member

IHttpContextAccessor is the way to do that yes. Ideally though you'd have another service that resolved the current tenant ID so that it's not coupled to the HttpContext, and there would be an Http based tenant resolver that would use the IHttpContextAccessor to grab the header.

@roji
Copy link
Member

roji commented Apr 9, 2022

IHttpContextAccessor is what I used in the sample (see discussion above), but I was surprised to learn that it's a singleton service (which would mean that scoped HTTP request information is retrieved via AsyncLocal or some similar mechanism?). Would have expected some scoped DI mechanism for retrieving HTTP request-bound data...

@davidfowl
Copy link
Member

There is no scoped service for accessing the HTTP request data.

@amiru3f
Copy link
Author

amiru3f commented Apr 10, 2022

Hello my friends and Appreciate the time you spent to review my issue.
@roji
As httpContextAccessor is singleton and retrieving tenant related data can be done via singleton object, isn't that more heap/memory friendly to make pooledDbContextFactory singleton?
And as you know it does not affect code readability too.

roji added a commit to roji/EntityFramework.Docs that referenced this issue Apr 17, 2022
To have a proper, scoped ITenant service.

Closes dotnet#3787
roji added a commit to roji/EntityFramework.Docs that referenced this issue Apr 17, 2022
To have a proper, scoped ITenant service.

Closes dotnet#3787
@roji
Copy link
Member

roji commented Apr 17, 2022

@amiru3f you're right that since HttpContextAccessor is a singleton service which internally uses AsyncLocal to access HTTP header data, it's possible to simply extend EF Core's PooledDbContextFactory, have it accept HttpContextAccessor and inject the tenant ID into contexts it hands out - while being registered as a Singleton itself.

However, the point of this code sample is to show how to manage context pooling when Scoped data is involved. Also, in most multi-tenant applications it would be inappropriate for EF's DbContext factory to directly access the HttpContext and perform tenant resolution, since tenant information is also needed elsewhere. So I've submitted #3822 to more closely approximate such a multitenant app, with a Scoped ITenant service.

@roji roji changed the title Service can be injected as singleton Update context pooling multitenancy sample to more closely approximate a real application Apr 17, 2022
@roji roji self-assigned this Apr 17, 2022
@roji roji added the area-perf label Apr 17, 2022
@roji roji added this to the 7.0.0 milestone Apr 17, 2022
roji added a commit to roji/EntityFramework.Docs that referenced this issue Apr 19, 2022
To have a proper, scoped ITenant service.

Closes dotnet#3787
roji added a commit that referenced this issue Apr 20, 2022
To have a proper, scoped ITenant service.

Closes #3787
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants