dotnet/aspnetcore

LoadAsync on Session - when _isNewSessionKey is true

Open

#20,898 opened on Apr 16, 2020

View on GitHub
 (10 comments) (0 reactions) (0 assignees)C# (10,653 forks)batch import
Perfaffected-fewarea-middlewarebreaking-changeenhancementhelp wantedseverity-nice-to-have

Repository metrics

Stars
 (37,933 stars)
PR merge metrics
 (Avg merge 16d 9h) (258 merged PRs in 30d)

Description

https://github.com/dotnet/aspnetcore/blob/8b000d961cd3ccfcc8090fb8368fd6598bace978/src/Middleware/Session/src/DistributedSession.cs

Should Load/LoadAsync be a no-op when _isNewSessionKey is true. Taking the advice to call LoadAsync prior to accessing the session with get/trygetvalue/set I would of thought we'd guard against unnecsssary chitchat to the cache if we know the session is new?

I though about doing Request.Cookies["sesscoookie"] != null then await LoadAsync but then doing Get/Set/Id/IsAvailable will simply call load anyway if not _loaded isnt marked.

Contributor guide

Research direction
Inspect the DistributedSession class and trace the LoadAsync logic when isNewSessionKey is true. Consider adding an early return to avoid unnecessary cache calls.
Tech stack
csharp
Domain
backend
Issue type
Bug
DifficultyEstimated implementation difficulty for a new contributor, from 1 for very small changes to 5 for expert-level work.
2
Estimated timeA rough time range for an experienced contributor to investigate, implement, test, and prepare a pull request.
1-3 hours
Activity statusHow available the issue appears right now: fresh, active, stale, blocked, or waiting on maintainer input.
Active
ClarityHow clearly the issue explains the expected change, acceptance criteria, and next step.
Clear
Prerequisites
C#ASP.NET Core
Newbie friendlinessA 1-100 score estimating how approachable this issue is for first-time contributors.
70