search

robinrodricks / FluentFTP

An FTP and FTPS client for .NET & .NET Standard, optimized for speed. Provides extensive FTP commands, File uploads/downloads, SSL/TLS connections, Automatic directory listing parsing, File hashing/checksums, File permissions/CHMOD, FTP proxies, FXP support, UTF-8 support, Async/await support, Powershell support and more. Written entirely in C#.
MIT License
3.14k stars 657 forks source link

Update to Async/Await Tasked-Based Asynchronous Programming Pattern #76

Closed jblacker closed 7 years ago

jblacker commented 7 years ago

I know that this library was designed with the old style of asynchronous programming using the Begin & End pattern. Is there currently any push to switch to the new async/await pattern?

If so, I'd be happy to help out rewriting the asynchronous methods. This library has helped me out a bunch and I'd like to contribute back if I could.

robinrodricks commented 7 years ago

Sure, your help would be greatly appreciated. If you don't have time to convert all the methods please feel free to submit a PR for whatever changes you have done, and I'll look and what you've done and repeat for the remaining methods..

jblacker commented 7 years ago

I started working on this, but I've been leaving the current Begin & End methods and wrapping them since they are already tested. All the new Task-based async methods will end with*Async(). I'm assuming you don't want this to be a breaking change right?

Additionally, I've been updating the documentation within the code as I go through it. Would you rather me submit the updated comments as a separate pull request or keep it together with this?

robinrodricks commented 7 years ago

I started working on this, but I've been leaving the current Begin & End methods and wrapping them since they are already tested.

Sounds okay. Perhaps later we can disable those/ make them private and see if the community is okay with that.

All the new Task-based async methods will end with*Async(). I'm assuming you don't want this to be a breaking change right?

Yes, try to avoid breaking changes.

Additionally, I've been updating the documentation within the code as I go through it. Would you rather me submit the updated comments as a separate pull request or keep it together with this?

Keep it together. Changing comments cannot cause any harm IMO :)

jblacker commented 7 years ago

@hgupta9 Regarding making those private / disabling them. I'd say the methods can probably be rewritten in a more "pure" asynchronous way sometime in the future. The wrapping will work, but it's kind of a quick & dirty way of allowing this to follow the Task-based async pattern. In the future these methods can probably be re-written async all the way down.

Here's my current plan. I'm up to step 3 and will be submitting a pull request once I finish this step and test everything out.

  1. Add overrides to the underlying stream sources (FtpDataStream & FtpSocketStream) to support the task-based async pattern. As of .NET 4.5 virtual methods already exist on the Stream abstract class so they just needed to be added to those two Stream implementations
  2. Wrap all of the current Begin & End asynchronous methods with Task.Factory.FromAsync() to produce an awaitable Task that encapsulates those methods.
  3. Add awaitable Task methods to the UploadFile / DownloadFile (and multi-versions)
  4. Rewrite all *Async methods to remove wrappers and use async/await pattern "all the way down". This would then depreciate the Begin & End methods.

Just a heads up regarding async/await. They were introduced in .NET 4.5. In order to prevent any breaking changes I added some additional build configurations for .NET 4 and a new preprocessor variable NETFX45. For all of the Task-based async methods I added a preprocessor directives to only build #if (CORE || NETFX45) I only have VS2013, so I can't update the projects for .NET Core, so you may need to update those to make sure the builds work for the correct targets

robinrodricks commented 7 years ago

Wow! Impressive! Can't wait for the PR.

jblacker commented 7 years ago

@hgupta9 Ha! Don't get too excited.

robinrodricks commented 7 years ago

Thank you so much.

robinrodricks commented 7 years ago

@jblacker Do you think your PR is ready for release? If you think it is I will create a release for nuget. I ran the tests you sent and they work fine.

robinrodricks commented 7 years ago

@jblacker Published on nuget - https://www.nuget.org/packages/FluentFTP/16.5.0

robinrodricks commented 7 years ago

How does your PR compare to this?

https://github.com/rkttu/System.Net.FtpClient.Async/blob/master/System.Net.FtpClient.Async/System.Net.FtpClient.Async/FtpClientAsyncExtension.cs

jblacker commented 7 years ago

@hgupta9 Never seen that library, but certain parts are very similar. However, not everything is since I went deeper than just adding async wrappers to the FtpClient methods. I do plan on unwrapping the methods and truly making them async all the way down in the near future.