Skip to content
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.

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

Remove custom cancellation token handling code, rely on Stream.ReadAsync to handle it instead #141

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

danzel
Copy link

@danzel danzel commented Sep 1, 2022

Fixes #131

I've tried tracing this code back in git history to figure out why this override exists.
Previously it had a comment:

We ensure that this service can be closed ASAP with the help of a Task.Delay.

Does Stream.ReadAsync take meaningfully longer to cancel than using a TaskCompletionSource?.

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.

@fubar-coder
Copy link
Contributor

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.

@danzel
Copy link
Author

danzel commented Sep 4, 2022

Is this something we need to support or test?
If you can let me know what needs manual testing I can hopefully do it.

@fubar-coder
Copy link
Contributor

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

@ite-klass
Copy link
Contributor

When I apply this change I can not connect via FTPS anymore (both explicit and implicit).

FileZilla client log:

Status:	TLS connection established.
Status:	Logged in
Status:	Retrieving directory listing...
Command:	PWD
Response:	257 "/"
Command:	TYPE I
Response:	200 Binary transfer mode active.
Command:	PASV
Response:	227 Entering Passive Mode (127,0,0,1,195,49).
Command:	MLSD
Response:	150 Opening data connection.
Error:	GnuTLS error -110 in gnutls_record_recv: The TLS connection was non-properly terminated.
Status:	Server did not properly shut down TLS connection
Error:	Could not read from transfer socket: ECONNABORTED - Connection aborted
Response:	226 Closing data connection.
Error:	Failed to retrieve directory listing

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.

Cancellation of FtpConnection.ReadFromStreamAsync causes UnobservedTaskException
3 participants