Skip to content

Refactor UdpSourceBase to use proper Dispose pattern #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
codegefluester merged 6 commits into from
Feb 10, 2026
Merged

Refactor UdpSourceBase to use proper Dispose pattern #7

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a857b78
Initial plan
Copilot Feb 10, 2026
a2446cb
Refactor UdpSourceBase to use Dispose pattern instead of manual dispo…
Copilot Feb 10, 2026
6d5b8e1
Add thread-safety to Dispose method in UdpSourceBase
Copilot Feb 10, 2026
50797a7
Move base.Dispose() inside lock for better thread-safety
Copilot Feb 10, 2026
11d98ff
Set disposed flag before disposal operations for better exception safety
Copilot Feb 10, 2026
d3c5fe4
Add try-finally to ensure base.Dispose() is always called
Copilot Feb 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion GamesDat/Telemetry/Sources/UdpSourceBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ namespace GamesDat.Core.Telemetry.Sources
{
public abstract class UdpSourceBase<T> : TelemetrySourceBase<T>
{
private readonly object _disposeLock = new object();
private bool _disposed;
Comment on lines +12 to +13
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

The _disposed flag check doesn't prevent ReadContinuousAsync from using a disposed _listener. If Dispose() is called while ReadContinuousAsync is running, the _disposed flag will be set to true, but the running async method will continue to attempt to use _listener on line 38, which has just been disposed on line 62.

There's no check of the _disposed flag before or during ReadContinuousAsync execution. Consider adding a check at the start of ReadContinuousAsync to throw ObjectDisposedException if already disposed, and potentially in the while loop to exit gracefully if disposal occurs during iteration.

Copilot uses AI. Check for mistakes.

protected UdpClient _listener;
protected IPEndPoint _endpoint;
Expand Down Expand Up @@ -45,7 +47,25 @@ public override async IAsyncEnumerable<T> ReadContinuousAsync([EnumeratorCancell
finally
{
_isListening = false;
_listener.Dispose();
}
}

public override void Dispose()
{
lock (_disposeLock)
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

The _disposeLock is used to synchronize Dispose() calls, but it doesn't protect against concurrent access between Dispose() and ReadContinuousAsync. The lock only prevents multiple Dispose() calls from running simultaneously, but doesn't prevent Dispose() from running while ReadContinuousAsync is accessing _listener on line 38.

If the intent is to allow Dispose() to be called from a different thread while ReadContinuousAsync is running (which is the typical pattern), then the _listener resource should not be disposed in the Dispose() method at all - it should only be disposed in the finally block of ReadContinuousAsync. The Dispose() method could instead set a cancellation flag that ReadContinuousAsync can check.

Copilot uses AI. Check for mistakes.
{
if (!_disposed)
{
_disposed = true;
try
{
_listener?.Dispose();
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

The null-conditional operator is unnecessary here because _listener is initialized in the constructor (line 28) and is never set to null. If this is meant to handle scenarios where the constructor throws before initialization, note that partial construction is not a concern since an exception in the constructor would prevent the object from being created and Dispose wouldn't be called.

However, if the recommendation from the first comment is followed (to move disposal back to the finally block), then this line should be removed entirely from the Dispose() method.

Suggested change
_listener?.Dispose();
_listener.Dispose();

Copilot uses AI. Check for mistakes.
}
finally
{
base.Dispose();
}
}
}
}
Comment on lines +53 to 70
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

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

Potential double-disposal issue: With the current implementation, if ReadContinuousAsync completes normally and then Dispose() is called, _listener will be disposed twice - once in the new Dispose() override (line 62) and potentially again if the pattern in the documentation is followed where finally blocks also dispose resources.

Additionally, this conflicts with the planned API refactoring documented in docs/telemetry-source-api-refactoring.md (lines 97-153), which shows that _listener.Dispose() should remain in the finally block even after the refactoring. The current change makes that future refactoring more complex.

UdpClient.Dispose() is idempotent (can be called multiple times safely), but this pattern is still confusing and unnecessary.

Copilot uses AI. Check for mistakes.

Expand Down
Loading