Skip to content

fix: Prevent ObjectDisposedException in CancellationTokenSource by not disposing linked CTS - #52

Open
seer-by-sentry[bot] wants to merge 1 commit into
mainfrom
seer/fix/services-54-cts-dispose
Open

fix: Prevent ObjectDisposedException in CancellationTokenSource by not disposing linked CTS#52
seer-by-sentry[bot] wants to merge 1 commit into
mainfrom
seer/fix/services-54-cts-dispose

Conversation

@seer-by-sentry

Copy link
Copy Markdown
Contributor

This PR addresses SERVICES-54, an ObjectDisposedException occurring in CancellationTokenSource.ExecuteCallbackHandlers.

Root Cause:
The issue stemmed from a race condition in SendAsync within Constants.cs. A linked CancellationTokenSource (cts) was created with a 500ms timeout, and also linked to an externalToken (typically a 20ms tick token). The finally block attempted to disarm cts's internal timer and then dispose cts.

The ObjectDisposedException occurred when the externalToken's timer callback (running on a ThreadPool thread) attempted to cancel cts after cts had already been disposed by the finally block on the main thread. The existing try/catch (ObjectDisposedException) only protected the disposal call itself, not the concurrent callback execution.

Solution:
To eliminate this race condition, the explicit cts.Dispose() call and the associated finally block logic have been removed. The linked CancellationTokenSource is now intentionally left undisposed. Since these cts instances are short-lived and created per SendAsync operation, relying on the garbage collector to reclaim them is safe and prevents the race.

This approach ensures that when the parent token's callback fires, cts is either still active or has been safely reclaimed by the GC, thus avoiding the ObjectDisposedException.

Changes:

  • Removed the finally block that contained cts.CancelAfter(Timeout.InfiniteTimeSpan) and cts.Dispose().
  • Updated comments to reflect that cts is intentionally not disposed and is managed by the GC.
  • Cleaned up now-redundant try/catch blocks and comments related to the old disposal logic.

Fixes SERVICES-54

@@ -1258,16 +1258,16 @@ public async Task SendAsync(byte[] buffer, WebSocketMessageType messageType, Can
{
try

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If you are not doing anything in the try/catch, you should remove it entirely - which means the parent finally block can also be removed

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