Persistence: remove ambient AsyncLocal context stack#795
Conversation
The EntityFramework persistence layer used an AsyncLocal-based context stack (ContextStack) so that shared singleton repositories could discover the current context, its DbContext and its GameConfiguration. Since every repository operation already originates from an EntityFrameworkContextBase (which holds its own DbContext and current GameConfiguration) and the stack never actually nested distinct contexts, the ambient mechanism was unnecessary. This replaces the stack by threading the originating context explicitly: - Remove ContextStack / IContextStack / IContextStackProvider. - Add an internal IContextAwareRepository (and context-aware GetRepository overloads on IContextAwareRepositoryProvider) so repositories receive the originating context as an explicit parameter. - Thread the originating context through GenericRepositoryBase, the configuration/account/letter/game-server repositories and CachedRepository, while keeping the shared per-GameConfiguration caches intact. - Move the cache/non-cache repository selection (edit TypedContext check) to read the explicitly passed context instead of the ambient stack. Public API (IPersistenceContextProvider, IRepositoryProvider, ICacheAwareRepositoryProvider) and the InMemory implementation are unchanged. https://claude.ai/code/session_015sGcQwyYMwppSjbP1uD66j
…ds must be internal CachedRepository<T> is a public class, so its public methods cannot expose the internal type EntityFrameworkContextBase. The two context-aware overloads added during the ambient-stack removal are changed from public to internal; they are called only within this assembly. https://claude.ai/code/session_015sGcQwyYMwppSjbP1uD66j
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the EntityFramework persistence layer by removing the ambient AsyncLocal context stack. By replacing implicit context management with explicit context threading, the system becomes more predictable and allocation-free. The changes ensure that the originating context is correctly passed through repository operations, while maintaining existing caching behaviors and public API compatibility. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the persistence layer by removing the ambient ContextStack mechanism in favor of explicitly passing an originating context (EntityFrameworkContextBase) through repositories and repository providers. While this is a positive architectural change, several critical issues were identified in the implementation. Specifically, ensuring caches on a temporary context in AccountRepository will crash when no explicit context is provided, and calling DeleteAsync in ConfigurationTypeRepository with a null context is guaranteed to throw an exception. Additionally, throwing exceptions on null contexts in CachingGameConfigurationRepository breaks the parameterless repository interface contracts. These issues should be addressed by implementing appropriate fallback context creation and handling null configurations gracefully.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| using var ownedContext = context is null ? this.GetContext(null) : null; | ||
| var origin = context ?? ownedContext!; | ||
|
|
||
| using var context = this.GetContext(); | ||
| await context.Context.Database.OpenConnectionAsync(cancellationToken).ConfigureAwait(false); | ||
| this.RepositoryProvider.EnsureCachesForCurrentGameConfiguration(origin); |
There was a problem hiding this comment.
If context is null, ownedContext is created as a temporary context. However, this temporary context will not have CurrentGameConfiguration set on its EntityDataContext. Calling EnsureCachesForCurrentGameConfiguration(origin) will subsequently throw an InvalidOperationException because CurrentGameConfiguration is null.
To prevent this crash when loading an account without an explicit context, we should only ensure caches if the context actually has a game configuration set.
using var ownedContext = context is null ? this.GetContext(null) : null;
var origin = context ?? ownedContext!;
if (origin.Context is EntityDataContext { CurrentGameConfiguration: not null })
{
this.RepositoryProvider.EnsureCachesForCurrentGameConfiguration(origin);
}| var gameConfiguration = this.GetCurrentGameConfiguration(null); | ||
| var collection = this._collectionSelector(gameConfiguration); | ||
| return collection.Remove(item); |
There was a problem hiding this comment.
Calling GetCurrentGameConfiguration(null) with a hardcoded null argument will always throw an InvalidOperationException because GetCurrentGameConfiguration expects a non-null context with an active game configuration. This makes DeleteAsync completely broken and guaranteed to crash at runtime.
If deleting configuration types is not supported, it is better to explicitly throw a NotSupportedException.
throw new NotSupportedException("Deleting configuration types directly through the repository is not supported.");| if (context is not { } currentContext) | ||
| { | ||
| throw new InvalidOperationException("There is no current context set."); | ||
| } |
There was a problem hiding this comment.
Throwing an InvalidOperationException when context is null breaks the public IRepository<GameConfiguration> interface contract, which allows parameterless calls.
Since CachingGameConfigurationRepository inherits from GenericRepositoryBase<GameConfiguration>, it can use the same fallback pattern as other repositories to create a temporary context when none is provided.
using var ownedContext = context is null ? this.GetContext(null) : null;
var currentContext = context ?? ownedContext!;| if (context is not { } currentContext) | ||
| { | ||
| throw new InvalidOperationException("There is no current context set."); | ||
| } |
There was a problem hiding this comment.
Throwing an InvalidOperationException when context is null breaks the public IRepository<GameConfiguration> interface contract, which allows parameterless calls.
Since CachingGameConfigurationRepository inherits from GenericRepositoryBase<GameConfiguration>, it can use the same fallback pattern as other repositories to create a temporary context when none is provided.
using var ownedContext = context is null ? this.GetContext(null) : null;
var currentContext = context ?? ownedContext!;- AccountRepository: guard EnsureCachesForCurrentGameConfiguration behind a CurrentGameConfiguration null-check so temporary contexts don't crash. - ConfigurationTypeRepository.DeleteAsync(object): throw NotSupportedException instead of calling GetCurrentGameConfiguration(null) which always throws. - CachingGameConfigurationRepository.GetByIdAsync/GetAllAsync: use the GetContext(null) fallback rather than throwing when context is null, so the parameterless IRepository<T> contract is honoured. - GameConfigurationRepository.GetByIdAsync/GetAllAsync: same fallback fix. https://claude.ai/code/session_015sGcQwyYMwppSjbP1uD66j
|
I'm not sure if I like this yet 😅 |
2 participants
$(cat <<'EOF'
Summary
Removes the
AsyncLocal<Stack<IContext>>ambient context stack (ContextStack,IContextStack,IContextStackProvider) from the EntityFramework persistence layer and replaces it with explicit context threading.Why the stack was unnecessary:
GameConfigurationis already stored onEntityDataContext.CurrentGameConfigurationand never changes during a context's lifetime.Stack<>depth was always ≤ 1.CachingGameConfigurationRepository.GetAllAsync,GameServerDefinitionRepository.LoadDependentDataAsync) mutate theEntityDataContextfield, not the stack.What changed:
ContextStack.cs,IContextStack.cs,IContextStackProvider.cs.IContextAwareRepository(internal interface) — context-awareGetByIdAsync/GetAllAsyncoverloads.GetRepository(Type, EntityFrameworkContextBase?)overloads toIContextAwareRepositoryProvider/CacheAwareRepositoryProvider/RepositoryProvider.UseContext(this)push/pop calls removed fromEntityFrameworkContextBaseandPlayerContext; the originating context is now passed explicitly.LoadDependentDataAsync→LoadCollectionAsync→LoadNavigationPropertyAsync.GameConfigurationcaches inConfigurationTypeRepository<T>remain shared singletons — the caching benefit is fully preserved.IPersistenceContextProvider,IRepositoryProvider,ICacheAwareRepositoryProvider) and the InMemory implementation are untouched.Result: simpler, allocation-free (no
AsyncLocal), and the data flow is explicit.Test plan
dotnet buildpasses (CI)MUnique.OpenMU.Persistence.Initialization.Testspass (CI)MUnique.OpenMU.Testspass (CI) — covers FriendServer, Guild, Trade, MoveItem scenariosEOF
)
Generated by Claude Code