dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.4k stars 10k forks source link

Introduce a session metadata to load session asynchronously in session middleware #35154

Open Kahbazi opened 3 years ago

Kahbazi commented 3 years ago

Background and Motivation

The default session provider in ASP.NET Core loads session records from the underlying IDistributedCache backing store asynchronously only if the ISession.LoadAsync method is explicitly called before the TryGetValue, Set, or Remove methods. If LoadAsync isn't called first, the underlying session record is loaded synchronously, which can incur a performance penalty at scale.

We could provide a new endpoint metadata with SessionState property for session which controls the behavior of SessionMiddleware.

Proposed API

public class SessionOptions
{
     // The behavior of session middleware when `ISessionStateMetadata` is not available.
+   public SessionState DefaultBehavior { get; set; }
}
public interface ISessionStateMetadata
{
    SessionState State { get; }
}

public enum SessionState
{
    Default = 0,
    EagerLoad = 1,
    Disabled = 2,
    ReadOnly = 3,
}

public class SessionStateAttribute : ISessionStateMetadata
{
    SessionState State { get; }
}

public static class RoutingEndpointConventionBuilderExtensions
{
    public static TBuilder SetSessionBehvior<TBuilder>(this TBuilder builder, SessionState sessionState) where TBuilder : IEndpointConventionBuilder;
}

Usage Examples

[SessionState(SessionState.Disabled)]
public class CustomerController
{
    ...
}

app.MapGet("/nosession", (HttpContext context) => $"Hello World!").SetSessionBehvior(SessionState.Disabled);

app.MapGet("/nosession2", [SessionState(SessionState.Disabled)] (HttpContext context) => $"Hello World!");

app.MapGet("/", (HttpContext context) => $"{context.Session.GetString("Hi")} World!").SetSessionBehvior(SessionState.ReadOnly);

Risks

If Endpoints are marked by mistake the session would load for all requests even when they don't need session.

davidfowl commented 3 years ago

Prior art in this area:

Some thoughts:

Kahbazi commented 3 years ago

Does Required=false mean don't load? Or just don't eagerly load? I'm trying to figure out if this is just a marker interface e.g. https://github.com/dotnet/aspnetcore/blob/ec504ed0130837873576de3d7eee804e4a0987f5/src/Http/Metadata/src/IAllowAnonymous.cs

If not loading session at all could lead to not setting the session feature, it is a good idea, it's useful for application that both have state-full UI and state-less rest API. Specially with the current implementation which always allocate and create session key.

I change the metadata and updated the proposal based on the https://docs.microsoft.com/en-us/dotnet/api/system.web.sessionstate.sessionstatebehavior?view=netframework-4.8.

davidfowl commented 3 years ago

I prefer a single method/attribute + enum:

[SessionStateAttribute(SessionState.Disabled)]
public class CustomerController
{
    ...
}

app.MapGet("/nosession", (HttpContext context) => $"Hello World!").SetSessionBehvior(SessionState.Disabled);

app.MapGet("/nosession2", [SessionStateAttribute(SessionState.Disabled)] (HttpContext context) => $"Hello World!");

app.MapGet("/", (HttpContext context) => $"{context.Session.GetString("Hi")} World!").SetSessionBehvior(SessionState.ReadOnly);
Kahbazi commented 3 years ago

I prefer a single method/attribute + enum:

I updated the proposal.

Kahbazi commented 3 years ago

@adityamandaleeka Any chance this could be done in .Net 6.0? I can send a PR for this feature when the proposal is approved.

davidfowl commented 3 years ago

This doesn't need to go into .NET 6. Even if you do the work, it needs to be reviewed by the team so it's not free. I suggest writing a middleware that does the same for now and can be shipped on nuget and we can revisit in .NET 7.

danirzv commented 1 year ago

I'm interested to help on this issue, may I send pull request?

mitchdenny commented 1 year ago

@danirzv thanks for the offer. I think before you picked this up we'd want to make sure that the shape of the API here is something that we are comfortable with. @davidfowl - this is a some what old API proposal - is it developed enough to go through API review?