Skip to content

Don't call socket task dispose - #1026

Open
Tar-Elendil wants to merge 5 commits into
connamara:masterfrom
Tar-Elendil:fix_thread_disposal
Open

Tar-Elendil wants to merge 5 commits into
connamara:masterfrom
Tar-Elendil:fix_thread_disposal

Conversation

@Tar-Elendil

@Tar-Elendil Tar-Elendil commented Jul 7, 2026

Copy link
Copy Markdown

This PR removes the direct call to Task.Dispose in QuickFIXn/SocketReader.cs
fixes #1025

The task does not always reach a completed state before the 1s timeout is reached and Dispose is called.
I don't believe that we strictly need to dispose the task here as the CancellationToken is cancelled. The task should complete eventually.

@CLAassistant

CLAassistant commented Jul 7, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@stic

stic commented Aug 18, 2026

Copy link
Copy Markdown

This PR removes the direct call to Task.Dispose in QuickFIXn/SocketReader.cs fixes #1025

The task does not always reach a completed state before the 1s timeout is reached and Dispose is called. I don't believe that we strictly need to dispose the task here as the CancellationToken is cancelled. The task should complete eventually.

That's true .Dispose() isn't needed since .net 4.5, just set it back to null? However, as is the change would eliminate the 1 second delay, isn't it?

Also, in SocketInittiatorThread you have the same pattern.

@Tar-Elendil

Tar-Elendil commented Aug 31, 2026

Copy link
Copy Markdown
Author

That's true .Dispose() isn't needed since .net 4.5, just set it back to null? However, as is the change would eliminate the 1 second delay, isn't it?

Also, in SocketInittiatorThread you have the same pattern.

Thanks I forgot about the Initiator.

Yes, there shouldn't be a performance impact and socket disconnects shouldn't be happening frequently.
I should've linked this blog.

You're right about the 1s delay. We need that to allow any existing wait op to complete.

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.

SocketReader task throws System.InvalidOperationException

3 participants