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

OpenRead and OpenWrite deprecated? #44

Closed
dex3r opened this issue Feb 3, 2017 · 8 comments
Closed

OpenRead and OpenWrite deprecated? #44

dex3r opened this issue Feb 3, 2017 · 8 comments

Comments

@dex3r
Copy link

dex3r commented Feb 3, 2017

OpenRead and OpenWrite are the only two ways to open stream to the file and return it (except async versions, which, I assume, going to be deprecated also). DownloadFile and UploadFile works fine in simple cases. However, when creating bigger API, returning Stream is the way to go.
In my case, I have FileSystem, File and Directory abstracion and in all cases (local file, network share, SFTP, Dokan) returning Stream works fine. It's simple and efficient. With FluentFTP and the new DownloadFile and UploadFile API the only way to implement this is to create MemoryStream, which is impossible for big files.

Is there an important reason to obsolete OpenRead and OpenWrite or not to create versions of DownloadFile and UploadFile where Stream is returned?

@robinrodricks
Copy link
Owner

DownloadFile and UploadFile also have stream versions. Does that work for you? You can upload any stream and download will return a MemoryStream.

OpenRead and OpenWrite cause many edge cases in my testing. It must be carefully closed, extra commansd, etc.

@dex3r
Copy link
Author

dex3r commented Feb 3, 2017

It may cause edge cases in your testing, but it's limiting. How? When you are passing Stream as parameter to write file to server, you have to know content of the file in the moment you are calling the method. This makes things like fly-by-copy impossible. You would have to download entire file to memory and then send it to the second server. If Stream was returned, instead of passing it a parameter, you could send the file regardles if you know the content beforehand or not.

As for reading files from server, there are cases like this too. One of them: you want to seek in the file and read only a part of it. With returned Stream it's easy. With Stream as parameter, you have to download entire file to disk (too large to hold in RAM) and then do the seek. This is very inefficient.

Those cases can be handled, but with some very cumbersome and hacky Stream implementations. No need to do this with returned Streams versions.

Here is another example. Consider two methods in some API that have the following signatures:

Stream OpenStream(string path, FileAccess access);
void OpenStream(Stream targetStream, string path, FileAccess access);

There is no way to use FluentFTP's methods DownloadFile and UploadFile to implement the first method.
However, if FluentFTP's methods were OpenRead and OpenWrite both of given methods could be implemented easly.

@robinrodricks
Copy link
Owner

Good points. I'll consider removing the Obsolete attribute.

@robinrodricks
Copy link
Owner

I've removed the Obsolete attribute from those methods. I added the attribute because I wanted older/existing users to know that DownloadFile and UploadFile methods are there and should be used in most cases. They have extra code like : reading the FtpReply at the end of the transfer to prevent "stale data" on the socket. This code will have to be manually added by the user.

@julian-goldsmith
Copy link

Can you expand on reading the FtpReply? I'm using OpenRead and OpenWrite in a project, and want to make sure I have all the edge cases covered.

@robinrodricks
Copy link
Owner

robinrodricks commented Feb 7, 2017

Basically after successfully transferring a file, the server sends back a message to indicate if the transfer was successful/failed. To read this just call GetReply like this. If you don't read this message, the next time you send a command it crashes with "stale data on socket".

Use the code in UploadFileInternal/DownloadFileInternal for reference. Also I found reading the file in chunks is a good idea, so perhaps you want to take the mentioned functions are fork them for your usage rather than starting from scratch.

@julian-goldsmith
Copy link

Awesome. I appreciate the help, and the work you've done on this library, Harsh.

@robinrodricks
Copy link
Owner

@julian-goldsmith - Thank you! Its very kind of you...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants