Skip to content

Comments

Fix RedisGrainDirectory uninitialize exception#9940

Open
miguelhasse wants to merge 3 commits intodotnet:mainfrom
miguelhasse:fix/redis-grain-directory-uninitialize-exception
Open

Fix RedisGrainDirectory uninitialize exception#9940
miguelhasse wants to merge 3 commits intodotnet:mainfrom
miguelhasse:fix/redis-grain-directory-uninitialize-exception

Conversation

@miguelhasse
Copy link
Contributor

@miguelhasse miguelhasse commented Feb 18, 2026

Updated the Uninitialize method to replace CloseAsync and synchronous Dispose calls with await _redis.DisposeAsync(), ensuring proper asynchronous disposal. Cleanup of _redis and _database fields is now handled in a finally block to guarantee resource release even if disposal fails.

This change is a fix to avoid NullReferenceException being thrown during calls to RedisGrainDirectory .Uninitialize

image
Microsoft Reviewers: Open in CodeFlow

Updated the Uninitialize method to replace CloseAsync and synchronous Dispose calls with await _redis.DisposeAsync(), ensuring proper asynchronous disposal. Cleanup of _redis and _database fields is now handled in a finally block to guarantee resource release even if disposal fails.
Copilot AI review requested due to automatic review settings February 18, 2026 07:00
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the Redis grain directory shutdown path to dispose the Redis connection asynchronously and ensure _redis/_database fields are cleared even if disposal throws, addressing a NullReferenceException during RedisGrainDirectory.Uninitialize.

Changes:

  • Replace CloseAsync() + synchronous Dispose() with await _redis.DisposeAsync().
  • Move cleanup of _redis and _database into a finally block to guarantee field cleanup.

Comment on lines +192 to +206
private async Task Uninitialize(CancellationToken arg)
{
if (_redis != null && _redis.IsConnected)
{
_disposed = true;

await _redis.CloseAsync();
_redis.Dispose();
_redis = null!;
_database = null!;
try
{
await _redis.DisposeAsync();
}
finally
{
_redis = null!;
_database = null!;
}
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change targets a shutdown-time exception path, but there’s no coverage ensuring Uninitialize is safe to call when initialization is partial/failed or when it’s invoked multiple times. Consider adding a Redis grain directory test which exercises the shutdown path (e.g., invoke Uninitialize via reflection or lifecycle and assert it doesn’t throw).

Copilot uses AI. Check for mistakes.
Removed IsConnected check before disposing _redis and added unsubscription from ConnectionRestored, ConnectionFailed, ErrorMessage, and InternalError event handlers to prevent memory leaks and ensure proper cleanup.
@ReubenBond
Copy link
Member

ReubenBond commented Feb 18, 2026

What is the source of the NullReferenceException in this case, as in: what is null here? From your exception, it looks like it's null on line 199 (_redis.Dispose();). If that's the case, are there overlapping calls to Uninitialize that are setting _redis to null? If that's an issue (it might be, I'm not sure yet), then it would likely be good to guard against that in code (eg, capture _redis and set it to null atomically, possibly via an interlocked operation or using a lock statement)

@miguelhasse
Copy link
Contributor Author

What is the source of the NullReferenceException in this case, as in: what is null here? From your exception, it looks like it's null on line 199 (_redis.Dispose();). If that's the case, are there overlapping calls to Uninitialize that are setting _redis to null? If that's an issue (it might be, I'm not sure yet), then it would likely be good to guard against that in code (eg, capture _redis and set it to null atomically, possibly via an interlocked operation or using a lock statement)

From the logged exception it look like it it thrown from inside StackExchange's implementation, but you may be right about the possible race condition. Using an interlocked operation might be a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants