Alternative interface for the AsyncFtpFolderMonitor

[adrianm64]

This PR contains an alternative interface for the AsyncFtpFolderMonitor.

Changes:

• The monitor is started with StartAsync and runs until the supplied CancellationToken is set or an exception occurs.
  This means you can add your own error handling and avoid a race condition on stop

while (true) {
    try {
	await monitor.StartAsync(token);
        break; // exited normally
    }
    catch (Exception ex) {
	Console.WriteLine("Something went wrong");
        // start again
}

• Async events are supported
  Unfortunately it means method calls to register event instead of +=.
  (async events can optionally have a CancellationToken argument)
  Multiple events are called in sequence, i.e. not parallel

monitor.FilesAdded(Monitor_FilesAddedAsync);
monitor.FilesChanged(Monitor_FilesChanged);

async Task Monitor_FilesAddedAsync(List<string> files) {
    await Task.Delay(TimeSpan.FromSeconds(5));
    Console.WriteLine($"{files.Count} files added: example {files[0]}");
}

void Monitor_FilesChanged(List<string> files) {
    Console.WriteLine($"{files.Count} files changed: example {files[0]}");
}

• Special PollInterval when unstable files are detected
  Shorter delay between a new file is detected and event is called when WaitTillFileFullyUploaded is true;

monitor.PollInterval = TimeSpan.FromMinutes(5);
monitor.WaitTillFileFullyUploaded = true;
monitor.UnstablePollInterval = TimeSpan.FromSeconds(10);

• Other minor changes (both sync and async version)

Intervals expressed in TimeSpan
ChangeDetected gets a list of all added/changed/deleted filenames.

• Things i would like to add but unsure of the best way

Pass the FtpClient to the events. Your event probably want to do something with the files.

Complete console application

using FluentFTP;
using FluentFTP.Monitors;

Console.WriteLine("Hello, World!");
var client = new AsyncFtpClient("server", "user", "password");

await client.Connect();

Console.WriteLine("Running FtpFolderMonitor");

var monitor = new AsyncFtpFolderMonitor(client, "Tests");

monitor.Recursive = false;
monitor.PollInterval = TimeSpan.FromSeconds(30);
monitor.WaitTillFileFullyUploaded = true;
monitor.UnstablePollInterval = TimeSpan.FromSeconds(5);

monitor.FilesAdded(Monitor_FilesAddedAsync);
monitor.FilesChanged(Monitor_FilesChanged);
monitor.FilesDeleted(Monitor_FilesDeleted);

async Task Monitor_FilesAddedAsync(List<string> files) {
    await Task.Delay(TimeSpan.FromSeconds(5));
    Console.WriteLine($"{files.Count} files added: example {files[0]}");
}
void Monitor_FilesChanged(List<string> files) {
    Console.WriteLine($"{files.Count} files changed: example {files[0]}");
}
void Monitor_FilesDeleted(List<string> files) {
    throw new Exception("Deleted called");
}

var cts = new CancellationTokenSource();
cts.CancelAfter(TimeSpan.FromSeconds(120));

try {
    await monitor.StartAsync(cts.Token);
}
catch (Exception ex) {
    Console.WriteLine("Something went wrong");
}

[adrianm64]

The failed checks is for code I didn't touch.

[robinrodricks]

Thanks for this huge update.

Question

However I don't understand why you are using the async version, then adding a while loop to essentially turn it into a fully synchronous program. In my understanding async methods should never be blocking (they should never contain perpetual while loops).

Why not just use the sync version then? Wouldnt this "alternate interface" be better addressed by simply disabling the timer feature on the existing sync FtpFolderMonitor and then adding a while loop in your user program?

try {
    await monitor.StartAsync(cts.Token);
}
catch (Exception ex) {
    Console.WriteLine("Something went wrong");
}

This usage indicates a blocking synchronous program, so I have no idea why async is used. You appear to be jumping through hoops just to get a simple cancellable while loop. What does this accomplish? What are the advantages of using async in this manner?

Changes

Can you please make the following changes if you wish for me to merge this.

1. Create a new class for this interface. Do not modify the original AsyncFtpFolderMonitor. Call your new class BlockingAsyncFtpFolderMonitor.
2. Remove UnstablePollInterval and just keep PollInterval. It makes no sense to have 2 properties when 1 suffices.
3. Move FtpFolderMonitorEventArgs to a new file (we don't use multiple classes per file by policy)
4. Use FtpFolderMonitorEventArgs even for the async version (currently its only implemented for sync version)
5. Rename StartAsync -> Start and PollFolderAsync -> PollFolder (we use this naming policy for the AsyncFtpClient so we should stick to the same policy here)
6. I don't see the need for anyone to add multiple event handlers to a single monitor. So please simplify the _filesChangedHandlers et al. to a single public callback property like public Func<FtpFolderMonitorEventArgs, CancellationToken, Task> OnFilesChanged, which would allow users of this class to set the property like monitor.OnFilesChanged = myCallback

[adrianm64]

It is not blocking, its async. Caller can just skip await and it will run fine in the "background".

var monitorTask = monitor.StartAsync(cts.Token);

// Continue program while monitor is running.

or if you want automatic restart on error

var monitorTask = RunUntilExit(cts.Token);
// Continue program while monitor is running and restarts on errors.
...

// at program exit
cts.Cancel();
await monitorTask;
monitor.Dispose();
...
async Task RunUntilExit(CancellationToken token) {
    while (true) {
        try {
            await _monitor.StartAsync(token);
            return;
        }
        catch {}
    }
}

I think this is the best way to handle errors from the monitor or from events. The current version just do Console.WriteLine and continue running. I assume you could add an Error event but that will, just like the other events, execute in the thread pool which can add complexity.
(StartAsync can run all events on the main thread)

It also avoids the nasty race condition when Stop is closing the connection while a timer event is running. Can be solved with ManualResetEvent or similar but adds complexity.

Your other points are valid even if I find two polling rates easier to understand (why does is it sometimes take ~15 minutes before I see new file if I have polling set to 10 minutes?) but it is not important.

[adrianm64]

public Func<FtpFolderMonitorEventArgs, CancellationToken, Task> OnFilesChanged

That is a bad idea. Multiple handlers can still be added with e.g. += and they need to be invoked by looping over the OnFilesChanged.GetInvokationList(). (invoking the delegate will run all handlers in parallel and only return the last Task).

If only one handler should be allowed, a method call SetHandler() or a constructor argument is better.
Might also be a good idea to combine all handlers into one ChangeDetected with all lists in the EventArgs.

[robinrodricks]

That is a bad idea.

See, I would still like just one event handler to be allowed and not multiple.

Your other points are valid

I will await your changes before I can merge it.

[FanDjango]

Thanks for these amendments. @robinrodricks will review this as his schedule allows.

[adrianm64]

Restored the previous code and reworked it into a new class instead. Don't think it should be merged as is but use it more as something to discuss.

Summary of changes:

• SetHandler to assign event handler
• Only one event which contain all changes
• Event has access to FtpClient and CancellationToken
• Pass entire FtpListItem to event instead of just file name
• Recursive is replaced with options. Works more like GetListing

[FanDjango]

Make sure you pick up #1668 to keep the code up to current master. Or we force push or something.

[FanDjango]

Don't worry too much about Codacity - it's a PITA. Robin will be back soon and then this can be taken forward.

[robinrodricks]

I'm normally very happy to receive every contribution and PR as I believe our contributors have been very instrumental in our success.

This PR however, had many points that irked me and did not go the way I wanted. So I was forced to change it myself.

Changes that I made

1. Used a special event args object with all the added/modified/deleted files
2. A single 'ChangeDetected' event is offered in all types of monitor classes (I removed the added/modified/deleted events)
3. Unified the codebase so all 3 types of monitors use the same base class
4. Removed detection of changed date in the blocking async monitor, kept it only filesize, same as other monitor classes
5. Removed listing of sub-folders in the blocking async monitor, kept it only for files, same as other monitor classes
6. Removed the UnstablePollInterval to simplify the API as much as possible
7. Reused base class code as far as possible in the blocking async monitor
8. Used TimeSpan instead of double (seconds) for the poll interval property in all monitors
9. All monitors support monitoring of multiple folders, rather than a single folder path

Naming

@adrianm64 Can you suggest a better name for your async monitor. I'm not happy with "blocking async monitor" as I do not think it reflects its behaviour correctly. As you are the original author, I'm giving you a chance to suggest a better name.

[adrianm64]

I'm normally very happy to receive every contribution and PR as I believe our contributors have been very instrumental in our success.

This PR however, had many points that irked me and did not go the way I wanted. So I was forced to change it myself.

Sorry about that.
I looked through your changes and I think they are good. The name is ok and I can't come up with anything better.

I made the class in a way that I can use in my day work and your version still qualifies for that. I build 30-40 different integrations between various business systems a year and maybe 30% of them use polling ftp/ftps/sftp in one end.

I would not use the regular monitor classes because:

1. No easy way to inject error handling/logging
2. Calling Stop might interrupt something
3. Doesn't fit well with the recommended AspNetCore and windows service way to run background tasks. (The "unusal async pattern" :-) BackgroundService)

Some small things:

• Constructor could use params but with collection initializers it doesn't really matter.
• A public field (or property) for the async delegate is troublesome and should have a warning
• There is a race condition in the handler call. Assign ChangeDetected to a local variable before checking for null and invoking.
• Someone is going to need file metadata in the handler. Having the FtpListItem instead of string in the event args could save a network call. (and the FtpListItem is there for free so why hide it?)

I will probably do a new PR with better examples and documentation.

[robinrodricks]

I looked through your changes and I think they are good. The name is ok and I can't come up with anything better.

Alright.

I made the class in a way that I can use in my day work and your version still qualifies for that. I build 30-40 different integrations between various business systems a year and maybe 30% of them use polling ftp/ftps/sftp in one end.

Good to know.

I would not use the regular monitor classes because

You could enhance them in a PR.

Someone is going to need file metadata in the handler. Having the FtpListItem instead of string in the event args could save a network call. (and the FtpListItem is there for free so why hide it?)

This is a good idea.

[robinrodricks]

Released.

https://www.nuget.org/packages/FluentFTP/52.0.0

FTP monitors are now documented here.

https://github.com/robinrodricks/FluentFTP/wiki/FTPS-Monitor