Refactor UdpSourceBase to use proper Dispose pattern

[Copilot]

Addresses feedback to replace manual disposal in finally block with proper IDisposable pattern.

Changes

• Moved resource cleanup to Dispose(): Removed _listener.Dispose() from ReadContinuousAsync finally block, now properly overrides Dispose() method
• Added thread-safety: Lock-based synchronization prevents race conditions during concurrent disposal
• Prevented double disposal: Disposed flag checked atomically before cleanup
• Exception safety: Try-finally ensures base.Dispose() executes even if listener disposal throws

Before

public override async IAsyncEnumerable<T> ReadContinuousAsync(...)
{
    try { /* ... */ }
    finally
    {
        _isListening = false;
        _listener.Dispose();  // Manual disposal in finally
    }
}

After

public override void Dispose()
{
    lock (_disposeLock)
    {
        if (!_disposed)
        {
            _disposed = true;
            try { _listener?.Dispose(); }
            finally { base.Dispose(); }
        }
    }
}

---

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

[Copilot]

Pull request overview

This PR attempts to improve the disposal pattern in UdpSourceBase<T> by moving _listener.Dispose() from the finally block in ReadContinuousAsync to an overridden Dispose() method. The changes add thread-safety mechanisms including a dispose lock and disposed flag to prevent double disposal.

Changes:

• Moved _listener.Dispose() from the finally block in ReadContinuousAsync to a new Dispose() override
• Added thread-safety with _disposeLock and _disposed flag
• Implemented try-finally in Dispose() to ensure base.Dispose() is always called

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Critical race condition: The removal of _listener.Dispose() from the finally block creates a scenario where the UdpClient could remain undisposed if ReadContinuousAsync completes naturally (via cancellation) but Dispose() is never called. This is a resource leak.

Additionally, there's a dangerous race condition where Dispose() can be called while _listener.ReceiveAsync(ct) on line 38 is in progress. When _listener.Dispose() executes on line 62 while a receive operation is active, it can cause unpredictable behavior including ObjectDisposedException being thrown into the async enumerable consumer.

The codebase pattern (see MemoryMappedFileSource.cs:38-42, TrackmaniaMemoryMappedFileSource.cs:125-131, and FileWatcherSourceBase.cs:205-212) is to dispose resources in the finally block of ReadContinuousAsync, not in a separate Dispose() method. This ensures proper cleanup regardless of how the async enumerable terminates.

Recommendation: Keep _listener.Dispose() in the finally block where it was originally. The override Dispose() method should call base.Dispose() only, and can optionally set a flag to prevent re-initialization if needed.

[Copilot]

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]

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]

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]

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.