Remove custom cancellation token handling code, rely on Stream.ReadAsync to handle it instead #141
Conversation
…ync to handle it instead
|
It might've been a problem in an older version of .NET (Core), that the stream would only be cancelled when new data arrives or some timeout occurs. |
|
Is this something we need to support or test? |
|
I thought that I read in an article (MS blog post), that they fixed it in .NET Core 3.1, but I might be misremembering it. You can read more about it here: https://devblogs.microsoft.com/dotnet/performance_improvements_in_net_7/#file-i-o |
This comment was marked as resolved.
This comment was marked as resolved.
|
Disregard my previous comment, which I have now hidden as resolved. Building this project source into an assembly resulted in ssl connect issues which do not occur after upgrading the project to net8, so that was unrelated to this change. |
|
We have 1.8k FTP clients connecting at close to the same time per day, and an online monitor that connects every 10 mins via TCP (no FTP interpretation). We saw a ton of unobserved TaskScheduler exceptions. I can now confirm that after applying this PR changeset to our net8-upgraded codebase, the unobserved TaskScheduler exceptions no longer occur, and I was able to cover the consequently await-thrown exceptions. They were connection-ending which is acceptable to a server. (client disconnects, unclean disconnects, etc) |
Fixes #131
I've tried tracing this code back in git history to figure out why this override exists.
Previously it had a comment:
Does
Stream.ReadAsynctake meaningfully longer to cancel than using aTaskCompletionSource?.If there is a reason it is this way, we can probably keep the behaviour and observe the result so that the we don't end up with an UnobservedTaskException later, it'll be a bit clunky though.