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

Make FtpStream::get read the final response #15

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

Conversation

oscarwcl
Copy link

@oscarwcl oscarwcl commented Apr 6, 2022

Once the client has finished reading a file from the server, the server
sends a status message to the client. The current API of FtpStream::get
doesn't allow us to read that response, since we return a BufReader that
reads directly from the data stream. Instead of doing this, we can
return a new future type that wraps the data stream and reads the
status message once the data stream is finished.

There is one caveat to this - if this future is dropped then the data
stream will be closed, and the server will send us an error message,
which still needs to be read manually. There isn't really a nice way
around this without an AsyncDrop trait.

Since this is already a breaking change, we also no longer use a
BufReader at all - this lets the caller decide whether to buffer or not.

This fixes #13

Once the client has finished reading a file from the server, the server
sends a status message to the client. The current API of FtpStream::get
doesn't allow us to read that response, since we return a BufReader that
reads directly from the data stream. Instead of doing this, we can
return a new AsyncRead type (FileReader) that wraps the data stream and
reads the status message once the data stream is finished.

There is one caveat to this - if the FileReader is dropped then the data
stream will be closed, and the server will send us an error message,
which still needs to be read manually. There isn't really a nice way
around this without an AsyncDrop trait.

Since this is already a breaking change, we also no longer use a
BufReader at all - this lets the caller decide whether to buffer or not.
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.

FtpStream::get() doesn't read status::CLOSING_DATA_CONNECTION
1 participant