Conversation
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 addresses several issues related to the SignalR client-side service, focusing on improving its robustness and encapsulation. It ensures proper initialization of SignalR connections in key UI components, handles event subscriptions more safely by allowing null event handlers, and refactors an internal retry policy class to be privately scoped within its containing service. These changes contribute to a more stable and maintainable client-side communication layer. Highlights
Changelog
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. 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
The pull request addresses multiple issues, including enabling nullable reference types, improving encapsulation by making a helper class private, and ensuring the SignalR service is initialized before use in components. My review focuses on the approach for initializing the SignalR service. While the current change is a valid fix, I've suggested a refactoring to improve the service's design and reduce code duplication in consumer components.
TeslaSolarCharger/Client/Components/StartPage/CarDetailsComponent.razor
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR addresses a set of small client-side SignalR reliability/cleanup issues in the Blazor client by tightening the SignalR state service contract and ensuring affected components initialize the SignalR connection before subscribing.
Changes:
- Make
InfiniteRetryPolicyan internal implementation detail ofSignalRStateService. - Mark
ISignalRStateService.OnConnectionStateChangedas nullable to match actual usage. - Ensure
CarDetailsComponentandChargingConnectorDetailsComponentcallInitializeAsync()before subscribing to SignalR state updates.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| TeslaSolarCharger/Client/Services/SignalRStateService.cs | Moves retry policy into the service as a private nested class. |
| TeslaSolarCharger/Client/Services/Contracts/ISignalRStateService.cs | Adjusts event nullability to align with nullable-enabled usage. |
| TeslaSolarCharger/Client/Components/StartPage/ChargingConnectorDetailsComponent.razor | Initializes SignalR connection before subscribing to state updates. |
| TeslaSolarCharger/Client/Components/StartPage/CarDetailsComponent.razor | Initializes SignalR connection before subscribing to state updates. |
Comments suppressed due to low confidence (2)
TeslaSolarCharger/Client/Components/StartPage/ChargingConnectorDetailsComponent.razor:100
- The callback passed to
Subscribe<...>is declared asasync void. Withasync void, exceptions after the firstawaitbypass the returnedTaskchain and can surface as unhandled, and the caller cannot await completion. Prefer a non-async callback (and dispatch async work via a safe fire-and-forget helper), or change the service API to acceptFunc<T, Task>so async callbacks can be awaited/observed.
await SignalRStateService.Subscribe<DtoChargingConnectorOverviewState>(
DataTypeConstants.ChargingConnectorOverviewState,
async void (state) =>
{
try
TeslaSolarCharger/Client/Components/StartPage/CarDetailsComponent.razor:193
- The
Subscribe<...>callback isasync void, which makes exceptions unobservable and prevents awaiting completion. Prefer a synchronous callback plus explicit safe dispatch, or update theISignalRStateService.Subscribecontract to acceptFunc<T, Task>so async handlers can be awaited and errors handled reliably.
await SignalRStateService.Subscribe<DtoCarOverviewState>(
DataTypeConstants.CarOverviewState,
async void (state) =>
{
try
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
TeslaSolarCharger/Client/Components/StartPage/ChargingConnectorDetailsComponent.razor
Outdated
Show resolved
Hide resolved
TeslaSolarCharger/Client/Components/StartPage/CarDetailsComponent.razor
Outdated
Show resolved
Hide resolved
- Added internal `await InitializeAsync()` calls to public methods in `SignalRStateService`. - Changed `Subscribe` and `SubscribeToTrigger` to return `Task<IDisposable>` for proper cleanup. - Removed redundant `InitializeAsync()` calls from Blazor components. - Added proper `IDisposable` implementation in components using SignalR subscriptions to prevent duplicate updates and memory leaks during re-renders. Co-authored-by: pkuehnel <[email protected]>
…1030221631243904585 refactor: Centralize SignalR initialization and fix subscription leaks
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant improvement by properly managing SignalR subscriptions in Blazor components using the IDisposable pattern. This will prevent memory leaks and other issues related to component lifecycle. The centralization of InitializeAsync in SignalRStateService also simplifies the component code.
My review focuses on a critical thread-safety issue in SignalRStateService that needs to be addressed. The rest of the changes look good.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
TeslaSolarCharger/Client/Components/StartPage/NotChargingAtExpectedPowerReasonsComponent.razor:137
OnParametersSetAsyncruns multiple times; this code overwrites_subscriptioneach time without disposing the previous one, leading to multiple trigger subscriptions and a leak. Dispose the existing subscription before re-subscribing, or only (re)subscribe when the effective key (e.g., CarId/ChargingConnectorId) changes.
protected override async Task OnParametersSetAsync()
{
await base.OnParametersSetAsync();
_subscription = await SignalRStateService.SubscribeToTrigger(
DataTypeConstants.NotChargingAsExpectedChangeTrigger,
async void () =>
{
try
{
await RefreshNotChargingWithExpectedPowerReasons();
await InvokeAsync(StateHasChanged).ConfigureAwait(false);
}
catch (Exception e)
{
Logger.LogError(e, "{method}() failed", nameof(RefreshNotChargingWithExpectedPowerReasons));
}
});
await RefreshNotChargingWithExpectedPowerReasons();
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
TeslaSolarCharger/Client/Components/StartPage/LoadpointComponent.razor
Outdated
Show resolved
Hide resolved
…rametersSetAsync Moved SignalR subscription logic from `OnInitializedAsync` to `OnParametersSetAsync` in `LoadpointComponent.razor`. By comparing the new `entityKey` with `_lastSubscribedKey`, the component correctly disposes the previous subscription and subscribes to the new topic whenever the Blazor framework reuses the component instance for a different LoadPoint (e.g. in list rendering). This prevents stale data binding and updates applying to the wrong row. Co-authored-by: pkuehnel <[email protected]>
…cle-8357064537335588501 Fix: Handle parameter changes in LoadpointComponent SignalR subscriptions
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant improvement by properly managing SignalR subscriptions using the IDisposable pattern across several Blazor components. This is a great step towards preventing memory leaks and redundant subscriptions. The related refactoring in SignalRStateService is mostly well-executed, but I've identified a couple of potential thread-safety issues regarding how subscriber lists are modified. My review comments include specific suggestions to address these concurrency concerns to make the implementation more robust. Overall, this is a valuable change that enhances the stability and performance of the client application.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
TeslaSolarCharger/Client/Components/StartPage/NotChargingAtExpectedPowerReasonsComponent.razor
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to resource management by implementing the IDisposable interface in several Blazor components and the SignalRStateService. This ensures that SignalR subscriptions are properly disposed of when components are no longer active, preventing potential memory leaks and improving application stability. The SignalRStateService has been enhanced to return IDisposable for subscriptions and to automatically initialize the connection when necessary, making it more robust. A new SubscriptionDisposable class has been added to facilitate this cleanup. Overall, these changes contribute to a more maintainable and efficient codebase.
TeslaSolarCharger/Client/Components/StartPage/ChargingConnectorDetailsComponent.razor
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
TeslaSolarCharger/Client/Components/StartPage/NotChargingAtExpectedPowerReasonsComponent.razor:134
OnParametersSetAsyncassigns_subscription = await ...SubscribeToTrigger(...)on every parameter update, overwriting the previous subscription without disposing it. This can create duplicate trigger subscriptions and leak callbacks until component disposal. Consider subscribing only once (e.g., when_subscription == null) or disposing the existing subscription before re-subscribing.
_subscription = await SignalRStateService.SubscribeToTrigger(
DataTypeConstants.NotChargingAsExpectedChangeTrigger,
async void () =>
{
try
{
await RefreshNotChargingWithExpectedPowerReasons();
await InvokeAsync(StateHasChanged).ConfigureAwait(false);
}
catch (Exception e)
{
Logger.LogError(e, "{method}() failed", nameof(RefreshNotChargingWithExpectedPowerReasons));
}
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to the SignalR state management by implementing a disposable subscription pattern, which prevents memory leaks and enhances robustness. The changes correctly handle component lifecycle events to manage subscriptions. Additionally, several threading issues in SignalRStateService are resolved, making the service more thread-safe. I've identified a potential race condition in GetStateAsync that could lead to inconsistent state on the client, which I've detailed in a specific comment.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
TeslaSolarCharger/Client/Services/SignalRStateService.cs:158
- Subscribe adds the callback to the in-memory subscriber list before EnsureSubscribedToDataType. If EnsureSubscribedToDataType throws (e.g., connection not established), the callback remains registered, which can leak memory and produce duplicate notifications if the caller retries. Wrap the subscription flow in try/catch and remove the callback on failure, or only add the callback after the data-type subscription succeeds.
public async Task<IDisposable> Subscribe<T>(string dataType, Action<T> callback, string entityId = "") where T : class
{
await InitializeAsync();
_logger.LogTrace("{method}<{type}>({dataType}, callback, {entityId})", nameof(Subscribe), typeof(T), dataType, entityId);
var key = _entityKeyGenerationHelper.GetDataKey(dataType, entityId);
Action<object> wrappedCallback = obj => { if (obj is T typedObj) callback(typedObj); };
// Add the callback
var subscribersList = _subscribers.GetOrAdd(key, _ => new List<Action<object>>());
lock (subscribersList)
{
subscribersList.Add(wrappedCallback);
}
// Subscribe to the data type if not already subscribed
await EnsureSubscribedToDataType(dataType);
TeslaSolarCharger/Client/Components/StartPage/NotChargingAtExpectedPowerReasonsComponent.razor:134
- OnParametersSetAsync re-subscribes every time it runs but overwrites _subscription without disposing the previous subscription. This can accumulate multiple active trigger subscriptions over time. Dispose the existing _subscription before assigning a new one (or subscribe once in OnInitialized/guard with a boolean).
private IDisposable? _subscription;
protected override async Task OnParametersSetAsync()
{
await base.OnParametersSetAsync();
_subscription = await SignalRStateService.SubscribeToTrigger(
DataTypeConstants.NotChargingAsExpectedChangeTrigger,
async void () =>
{
try
{
await RefreshNotChargingWithExpectedPowerReasons();
await InvokeAsync(StateHasChanged).ConfigureAwait(false);
}
catch (Exception e)
{
Logger.LogError(e, "{method}() failed", nameof(RefreshNotChargingWithExpectedPowerReasons));
}
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
TeslaSolarCharger/Client/Components/StartPage/LoadpointComponent.razor
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements by fixing memory leaks related to SignalR subscriptions and enhancing the thread safety of SignalRStateService. The use of IDisposable for subscriptions and robust handling of component lifecycles is well-implemented. I've found a potential race condition in the InitializeAsync method's logic that could cause issues after a successful auto-reconnection following a failed connection attempt. My review includes a suggestion to address this.
| try | ||
| { | ||
| if (_hubConnection != null) | ||
| // 1. Create the hub connection object if it doesn't exist yet | ||
| if (_hubConnection == null) | ||
| { | ||
| return; | ||
| } | ||
| _hubConnection = new HubConnectionBuilder() | ||
| .WithUrl(_navigationManager.ToAbsoluteUri("/appStateHub")) | ||
| .WithAutomaticReconnect(new InfiniteRetryPolicy(5)) | ||
| .Build(); | ||
|
|
||
| _hubConnection = new HubConnectionBuilder() | ||
| .WithUrl(_navigationManager.ToAbsoluteUri("/appStateHub")) | ||
| .WithAutomaticReconnect(new InfiniteRetryPolicy(5)) | ||
| .Build(); | ||
| _hubConnection.On<StateUpdateDto>(nameof(IAppStateClient.ReceiveStateUpdate), HandleStateUpdate); | ||
| _hubConnection.On<string, string>(nameof(IAppStateClient.ReceiveInitialState), HandleInitialState); | ||
|
|
||
| _hubConnection.On<StateUpdateDto>(nameof(IAppStateClient.ReceiveStateUpdate), HandleStateUpdate); | ||
| _hubConnection.On<string, string>(nameof(IAppStateClient.ReceiveInitialState), HandleInitialState); | ||
| _hubConnection.Reconnecting += (error) => | ||
| { | ||
| _logger.LogWarning(error, "SignalR connection lost, reconnecting..."); | ||
| OnConnectionStateChanged?.Invoke(); | ||
| return Task.CompletedTask; | ||
| }; | ||
|
|
||
| _hubConnection.Reconnecting += (error) => | ||
| { | ||
| _logger.LogWarning(error, "SignalR connection lost, reconnecting..."); | ||
| OnConnectionStateChanged?.Invoke(); | ||
| return Task.CompletedTask; | ||
| }; | ||
| _hubConnection.Reconnected += async (connectionId) => | ||
| { | ||
| _logger.LogInformation("SignalR reconnected with ID: {ConnectionId}", connectionId); | ||
| await ResubscribeToAllDataTypes(); | ||
| await RefreshAllStates(); | ||
| OnConnectionStateChanged?.Invoke(); | ||
| }; | ||
|
|
||
| _hubConnection.Closed += (error) => | ||
| { | ||
| _logger.LogWarning(error, "SignalR connection closed"); | ||
| OnConnectionStateChanged?.Invoke(); | ||
| return Task.CompletedTask; | ||
| }; | ||
| } | ||
|
|
||
| _hubConnection.Reconnected += async (connectionId) => | ||
| // 2. Start the connection ONLY if it is completely disconnected. | ||
| if (_hubConnection.State == HubConnectionState.Disconnected) | ||
| { | ||
| _logger.LogInformation("SignalR reconnected with ID: {ConnectionId}", connectionId); | ||
| // Assign the running task to our tracking variable | ||
| _connectionTask = StartConnectionInternalAsync(); | ||
| } | ||
|
|
||
| await ResubscribeToAllDataTypes(); | ||
| await RefreshAllStates(); | ||
| OnConnectionStateChanged?.Invoke(); | ||
| }; | ||
| // 3. Capture the task locally so we can await it outside the lock | ||
| taskToAwait = _connectionTask; | ||
| } | ||
| finally | ||
| { | ||
| _connectionLock.Release(); | ||
| } |
There was a problem hiding this comment.
There's a potential race condition here. If a manual connection attempt via InitializeAsync fails, _connectionTask will hold a faulted task. If SignalR's automatic reconnect then succeeds, the hub state will become Connected. However, the next call to InitializeAsync will not update the faulted _connectionTask, and will attempt to await it, causing an exception even though the connection is healthy.
To fix this, we should check if the connection is already Connected and reset the _connectionTask if it's in a faulted state before proceeding.
try
{
// 1. Create the hub connection object if it doesn't exist yet
if (_hubConnection == null)
{
_hubConnection = new HubConnectionBuilder()
.WithUrl(_navigationManager.ToAbsoluteUri("/appStateHub"))
.WithAutomaticReconnect(new InfiniteRetryPolicy(5))
.Build();
_hubConnection.On<StateUpdateDto>(nameof(IAppStateClient.ReceiveStateUpdate), HandleStateUpdate);
_hubConnection.On<string, string>(nameof(IAppStateClient.ReceiveInitialState), HandleInitialState);
_hubConnection.Reconnecting += (error) =>
{
_logger.LogWarning(error, "SignalR connection lost, reconnecting...");
OnConnectionStateChanged?.Invoke();
return Task.CompletedTask;
};
_hubConnection.Reconnected += async (connectionId) =>
{
_logger.LogInformation("SignalR reconnected with ID: {ConnectionId}", connectionId);
await ResubscribeToAllDataTypes();
await RefreshAllStates();
OnConnectionStateChanged?.Invoke();
};
_hubConnection.Closed += (error) =>
{
_logger.LogWarning(error, "SignalR connection closed");
OnConnectionStateChanged?.Invoke();
return Task.CompletedTask;
};
}
// If a previous connection attempt failed but auto-reconnect succeeded, the connection is now 'Connected'
// but '_connectionTask' might still be faulted. We should not await a faulted task if the connection is healthy.
if (_hubConnection.State == HubConnectionState.Connected && _connectionTask?.IsFaulted == true)
{
_connectionTask = null; // Reset the faulted task, so we don't await it.
}
// 2. Start the connection ONLY if it is completely disconnected.
if (_hubConnection.State == HubConnectionState.Disconnected)
{
// Assign the running task to our tracking variable
_connectionTask = StartConnectionInternalAsync();
}
// 3. Capture the task locally so we can await it outside the lock
taskToAwait = _connectionTask;
}
finally
{
_connectionLock.Release();
}There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 5. Throw a clear error if the connection is Reconnecting or failed, | ||
| // satisfying the requirement that it only returns when fully usable. | ||
| if (_hubConnection?.State != HubConnectionState.Connected) | ||
| { | ||
| throw new InvalidOperationException($"SignalR is not usable. Current state: {_hubConnection?.State}"); | ||
| } |
There was a problem hiding this comment.
InitializeAsync throws whenever _hubConnection.State is not Connected after awaiting _connectionTask. During automatic reconnect (HubConnectionState.Reconnecting/Connecting), _connectionTask may already be completed from the original start, so callers can get an InvalidOperationException even though the connection may recover moments later. Instead of throwing on Reconnecting, consider awaiting a reconnection completion signal (or polling with a timeout) and only throwing after an actual start/reconnect failure.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces crucial fixes for memory leaks in several Blazor components by correctly managing SignalR subscriptions. Implementing IDisposable and ensuring subscriptions are disposed of is a great improvement. The refactoring of SignalRStateService to improve thread safety and connection initialization robustness is also excellent.
I've found one issue in NotChargingAtExpectedPowerReasonsComponent where subscriptions could still leak, and I've left a comment with a suggestion to fix it.
Overall, these are very good changes that will improve the stability and performance of the application.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
TeslaSolarCharger/Client/Components/StartPage/NotChargingAtExpectedPowerReasonsComponent.razor:134
OnParametersSetAsyncre-subscribes every time parameters are set and overwrites_subscriptionwithout disposing the previous one. This can leak subscriptions and cause duplicate callbacks/state refreshes. Dispose the existing_subscriptionbefore re-subscribing, or move the subscription toOnInitialized{Async}and only resubscribe when relevant parameters (CarId/ChargingConnectorId) actually change.
protected override async Task OnParametersSetAsync()
{
await base.OnParametersSetAsync();
_subscription = await SignalRStateService.SubscribeToTrigger(
DataTypeConstants.NotChargingAsExpectedChangeTrigger,
async void () =>
{
try
{
await RefreshNotChargingWithExpectedPowerReasons();
await InvokeAsync(StateHasChanged).ConfigureAwait(false);
}
catch (Exception e)
{
Logger.LogError(e, "{method}() failed", nameof(RefreshNotChargingWithExpectedPowerReasons));
}
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (_hubConnection?.State != HubConnectionState.Connected) | ||
| { | ||
| throw new InvalidOperationException($"SignalR is not usable. Current state: {_hubConnection?.State}"); | ||
| } |
There was a problem hiding this comment.
InitializeAsync throws if the hub state isn’t exactly Connected after leaving the lock. If the connection is in Connecting/Reconnecting (e.g., during automatic reconnect), this will raise an InvalidOperationException and can break callers like GetStateAsync/Subscribe* during transient network hiccups. Consider waiting for the connection to reach Connected (e.g., via a TaskCompletionSource tied to Reconnected/Closed), or treat Connecting/Reconnecting as a non-error and return without throwing (leaving subsequent operations to retry).
| if (_hubConnection?.State != HubConnectionState.Connected) | |
| { | |
| throw new InvalidOperationException($"SignalR is not usable. Current state: {_hubConnection?.State}"); | |
| } | |
| // Only treat a definitively unusable connection as an error. Allow Connecting/Reconnecting | |
| // states to proceed without throwing so callers can retry during transient hiccups. | |
| if (_hubConnection == null || _hubConnection.State == HubConnectionState.Disconnected) | |
| { | |
| throw new InvalidOperationException($"SignalR is not usable. Current state: {_hubConnection?.State}"); | |
| } | |
| if (_hubConnection.State != HubConnectionState.Connected) | |
| { | |
| _logger.LogWarning("SignalR connection not yet fully established. Current state: {State}", _hubConnection.State); | |
| } |
| private bool _disposed; | ||
| public SubscriptionDisposable(Action unsubscribeAction) | ||
| { | ||
| _unsubscribeAction = unsubscribeAction ?? throw new ArgumentNullException(nameof(unsubscribeAction)); | ||
| } | ||
| public void Dispose() | ||
| { | ||
| if (_disposed) | ||
| { | ||
| return; | ||
| } | ||
| _disposed = true; |
There was a problem hiding this comment.
SubscriptionDisposable.Dispose() isn’t thread-safe: if Dispose() is called concurrently, _unsubscribeAction() can run multiple times because _disposed isn’t updated atomically. Consider using Interlocked.Exchange (or a lock) to ensure the unsubscribe action is executed at most once.
| private bool _disposed; | |
| public SubscriptionDisposable(Action unsubscribeAction) | |
| { | |
| _unsubscribeAction = unsubscribeAction ?? throw new ArgumentNullException(nameof(unsubscribeAction)); | |
| } | |
| public void Dispose() | |
| { | |
| if (_disposed) | |
| { | |
| return; | |
| } | |
| _disposed = true; | |
| private int _disposed; // 0 = not disposed, 1 = disposed | |
| public SubscriptionDisposable(Action unsubscribeAction) | |
| { | |
| _unsubscribeAction = unsubscribeAction ?? throw new ArgumentNullException(nameof(unsubscribeAction)); | |
| } | |
| public void Dispose() | |
| { | |
| if (System.Threading.Interlocked.Exchange(ref _disposed, 1) == 1) | |
| { | |
| return; | |
| } |
| return new SubscriptionDisposable(() => | ||
| { | ||
| if (_subscribers.TryGetValue(key, out var list)) | ||
| { | ||
| lock (list) | ||
| { | ||
| list.Remove(wrappedCallback); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Unsubscribing only removes the callback from the list but leaves the (potentially empty) list stored in _subscribers. Over time this can retain keys indefinitely and also keeps TryGetValue/ContainsKey behavior “true” even when there are no subscribers. Consider removing the dictionary entry when the list becomes empty after removal (e.g., inside the lock).
| { | ||
| lock (list) | ||
| { | ||
| list.Remove(callback); |
There was a problem hiding this comment.
Unsubscribing from _triggerSubscribers removes the callback but leaves an empty list in the dictionary, so _triggerSubscribers.ContainsKey(key) can remain true even when there are no trigger subscribers. Consider removing the key when the list becomes empty to avoid unbounded growth and to keep trigger-detection accurate.
| list.Remove(callback); | |
| list.Remove(callback); | |
| if (list.Count == 0) | |
| { | |
| _triggerSubscribers.TryRemove(key, out _); | |
| } |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to the SignalR state management, making it more robust and resilient. The introduction of IDisposable subscriptions and proper lifecycle management in components is a great step towards preventing memory leaks. The connection handling in SignalRStateService is also much more advanced now, with non-blocking initialization and background retries. I've found one issue related to subscription handling in one of the components, which I've detailed in a specific comment.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
TeslaSolarCharger/Client/Components/StartPage/NotChargingAtExpectedPowerReasonsComponent.razor:134
OnParametersSetAsync()assigns a new_subscriptioneach time it runs, but it doesn't dispose the previous subscription before overwriting. SinceOnParametersSetAsync()can run multiple times (e.g., whenCarId/ChargingConnectorIdchange), this can accumulate multiple active trigger subscriptions and leak callbacks. Dispose the old_subscription(or subscribe once inOnInitialized[Async]if appropriate) before creating a new one.
protected override async Task OnParametersSetAsync()
{
await base.OnParametersSetAsync();
_subscription = await SignalRStateService.SubscribeToTrigger(
DataTypeConstants.NotChargingAsExpectedChangeTrigger,
async void () =>
{
try
{
await RefreshNotChargingWithExpectedPowerReasons();
await InvokeAsync(StateHasChanged).ConfigureAwait(false);
}
catch (Exception e)
{
Logger.LogError(e, "{method}() failed", nameof(RefreshNotChargingWithExpectedPowerReasons));
}
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (_isRetryingInitialConnection) return; | ||
| _isRetryingInitialConnection = true; |
There was a problem hiding this comment.
StartBackgroundRetryAsync() uses _isRetryingInitialConnection as a non-atomic guard (if then set). If multiple callers hit this concurrently, it can start multiple background retry loops. Use an atomic guard (e.g., Interlocked.CompareExchange) or a lock to ensure only one retry task can be started at a time.
| if (_isRetryingInitialConnection) return; | |
| _isRetryingInitialConnection = true; | |
| lock (this) | |
| { | |
| if (_isRetryingInitialConnection) return; | |
| _isRetryingInitialConnection = true; | |
| } |
| _logger.LogWarning(ex, "Failed to establish initial SignalR connection. Starting background retry..."); | ||
|
|
||
| // Fire-and-forget the retry task. Notice the discard (_) | ||
| // We do NOT await this, so the lock in InitializeAsync is released immediately. | ||
| _ = StartBackgroundRetryAsync(); | ||
| } |
There was a problem hiding this comment.
StartConnectionInternalAsync() fire-and-forgets StartBackgroundRetryAsync() without observing the returned task. If StartBackgroundRetryAsync() throws (e.g., ObjectDisposedException from _connectionLock.WaitAsync() during shutdown), it becomes an unobserved exception. Consider catching/handling exceptions inside StartBackgroundRetryAsync() (especially cancellation/dispose) and/or tracking the retry task so it can be awaited/cancelled during DisposeAsync().
| private const int ConnectionRetryIntervallMilliseconds = 5000; | ||
| private bool _isRetryingInitialConnection = false; |
There was a problem hiding this comment.
The identifier ConnectionRetryIntervallMilliseconds has a spelling error (Intervall). Renaming to ConnectionRetryIntervalMilliseconds (and updating usages) would improve readability and avoid propagating the typo.
No description provided.