Fix Dapr client startup deadlock in distributed deployment#713
Fix Dapr client startup deadlock in distributed deployment#713
Conversation
… to StartedAsync Co-authored-by: sven-n <5238610+sven-n@users.noreply.github.com> Agent-Logs-Url: https://github.com/MUnique/OpenMU/sessions/b9f6bd43-e155-4f43-ad0d-a5b304a95c1b
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 resolves a critical startup deadlock issue in distributed deployments of the MUnique OpenMU project. The changes ensure that Dapr clients initialize correctly by decoupling the availability of the HTTP API from the readiness of game servers. This is achieved through the use of Highlights
🧠 New Feature in Public Preview: You can now enable Memory 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 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 counter productive. 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
|
There was a problem hiding this comment.
Code Review
This pull request effectively resolves a startup deadlock in distributed deployments by refactoring hosted services to use IHostedLifecycleService. This defers database-dependent initializations until after the web server has started, breaking the circular dependency with the Dapr sidecar. The changes are consistent across multiple services and follow best practices for handling service startup dependencies in ASP.NET Core. I've identified a couple of areas for improvement regarding the robustness of plugin loading and a potential memory leak in an event subscription, for which I've provided suggestions.
| var server = this._serviceProvider.GetRequiredService<IManageableServer>(); | ||
| this._data = new ServerStateData(server); | ||
| server.PropertyChanged += this.OnPropertyChanged; | ||
| this._server = server; |
There was a problem hiding this comment.
There is a potential issue here with multiple subscriptions to the PropertyChanged event. If an exception occurs after GetRequiredService but before this._server is set (e.g., in the ServerStateData constructor), the catch block will cause a retry. In the next iteration, GetRequiredService will return the same singleton IManageableServer instance, and another event handler will be subscribed. This leads to a memory leak and the handler being called multiple times for each event. It's safer to unsubscribe before subscribing to prevent this.
var server = this._serviceProvider.GetRequiredService<IManageableServer>();
server.PropertyChanged -= this.OnPropertyChanged; // Ensure single subscription
server.PropertyChanged += this.OnPropertyChanged;
this._data = new ServerStateData(server);
this._server = server;| if (this._serviceProvider.GetService<ICollection<PlugInConfiguration>>() is List<PlugInConfiguration> plugInConfigurations) | ||
| { | ||
| await this._serviceProvider.TryLoadPlugInConfigurationsAsync(plugInConfigurations).ConfigureAwait(false); | ||
| } |
There was a problem hiding this comment.
This conditional check on the type of the ICollection<PlugInConfiguration> could lead to silent failures. If the dependency injection is ever configured to provide a different type of collection, or if the service is not registered, this block will be skipped, and plugins won't be loaded, which could be hard to debug. A more robust approach would be to enforce the dependency on List<PlugInConfiguration> and fail fast if the assumption is not met.
var plugInCollection = this._serviceProvider.GetRequiredService<ICollection<PlugInConfiguration>>();
if (plugInCollection is not List<PlugInConfiguration> plugInConfigurations)
{
throw new InvalidOperationException($"The registered {nameof(ICollection<PlugInConfiguration>)} must be a {nameof(List<PlugInConfiguration>)} to be able to load plugin configurations.");
}
await this._serviceProvider.TryLoadPlugInConfigurationsAsync(plugInConfigurations).ConfigureAwait(false);| if (this._serviceProvider.GetService<ICollection<PlugInConfiguration>>() is List<PlugInConfiguration> plugInConfigurations) | ||
| { | ||
| await this._initializer.InitializeAsync().ConfigureAwait(false); | ||
| this._isInitialized = true; | ||
| await this._serviceProvider.TryLoadPlugInConfigurationsAsync(plugInConfigurations).ConfigureAwait(false); | ||
| } |
There was a problem hiding this comment.
This conditional check on the type of the ICollection<PlugInConfiguration> could lead to silent failures. If the dependency injection is ever configured to provide a different type of collection, or if the service is not registered, this block will be skipped, and plugins won't be loaded, which could be hard to debug. A more robust approach would be to enforce the dependency on List<PlugInConfiguration> and fail fast if the assumption is not met.
var plugInCollection = this._serviceProvider.GetRequiredService<ICollection<PlugInConfiguration>>();
if (plugInCollection is not List<PlugInConfiguration> plugInConfigurations)
{
throw new InvalidOperationException($"The registered {nameof(ICollection<PlugInConfiguration>)} must be a {nameof(List<PlugInConfiguration>)} to be able to load plugin configurations.");
}
await this._serviceProvider.TryLoadPlugInConfigurationsAsync(plugInConfigurations).ConfigureAwait(false);|
@copilot fix the issues described by gemini. |
…plugin config type Co-authored-by: sven-n <5238610+sven-n@users.noreply.github.com> Agent-Logs-Url: https://github.com/MUnique/OpenMU/sessions/9f61bfbb-6724-44c3-a7bb-c52f3b6ddf2e
Fixed in commit
|
WaitForDatabaseInitializationAsyncextension onIServiceProviderinExtensions.cs(with cancellation token support viaWaitAsync)ConnectServerHostedServiceWrapperto implementIHostedLifecycleService, acceptIServiceProviderinstead ofConnectServer, defer server start toStartedAsyncGameServerHostedServiceWrapperto implementIHostedLifecycleService, acceptIServiceProvider, moved plugin config loading + adapter initialization + server start toStartedAsyncManagableServerStatePublisherto implementIHostedLifecycleService, acceptIServiceProviderinstead ofIManageableServer, with separateInitializeServerAsyncto avoid duplicate event subscriptionsChatServerHostedServiceWrapperas new class inChatServer.Host, replacing the factory-based hosted service registrationConnectServer.Host/Program.cs: replaced blockingWaitForUpdatedDatabaseAsyncwith non-blockingWaitForDatabaseConnectionInitializationAsyncGameServer.Host/Program.cs: same, removed additional initialization calls (moved toGameServerHostedServiceWrapper.StartedAsync)ChatServer.Host/Program.cs: replaced factoryAddHostedServicewithAddHostedService<ChatServerHostedServiceWrapper>, removed blocking DB waitFriendServer.Host/Program.cs: replaced blockingWaitForUpdatedDatabaseAsyncwith non-blockingWaitForDatabaseConnectionInitializationAsyncGuildServer.Host/Program.cs: same as FriendServerPropertyChangedinInitializeServerAsyncto prevent duplicate event subscriptions on retryis List<PlugInConfiguration>checks inChatServerHostedServiceWrapperandGameServerHostedServiceWrapperwith fail-fast pattern (throwInvalidOperationExceptionif wrong type)Original prompt
⌨️ Start Copilot coding agent tasks without leaving your editor — available in VS Code, Visual Studio, JetBrains IDEs and Eclipse.