[fswatcher] File Reconciler w/ Resync Interval

[onelapahead]

Introduces a new Reconcile function to compliment Watch in fswatcher. Useful if in addition to file changes, you want to occasionally run the same, or another, function that consumes the file's state.

Note, Watch functionality essentially stays the same since onSync is always nil.

[EnriqueL8]

Would love to see an example on how you plan to use this onSync() method from the caller

[EnriqueL8]

I'm confused to see the onSync() function only being called if we hit the sync timeout but not when change is detected for example. I'm struggling to see the usefulness of an onSync() that takes zero arguments of when it synced to the calling function over an onChange that actually tells the consume that the file has changed.

[onelapahead]

But thats the thing - onChange also takes in no args, it expects the consumer to actually read the file themselves and do what they want with it.

[EnriqueL8]

Yeah, we can have a follow up issue to actually get the contents of the file back

[peterbroadhurst]

An alternative to this approach that we use a few places in FireFly codebases, is to wrap the context.
Generally it's a good idea for all for { select .... loops to include <-ctx.Done() so they will end if the context is terminated. This loop doesn't have that - which isn't caused by you, but does seem like an omission.

var cancelLoopCtx context.CancelFunc
tidyUpLoopCtx := func() {
  if cancelLoopCtx != nil {
       cancelLoopCtx();
   }
   cancelLoopCtx = nil;
}
for {
   loopCtx := ctx
   if resyncInterval != nil {
     loopCtx, cancelLoopCtx = context.WithTimeout(ctx, *resyncInterval)
  }
  select {
  ...
  case <-loopCtx.Done():
     // either we're exiting, or the timer popped
     select {
     case <-ctx.Done():
         tidyUpLoopCtx();
         log.L(ctx).Infof("exiting");
     default: // resync timer popped
     }
     tidyUpLoopCtx();
     // do the actual stuff.
  }

}

[dwertent]

I’d like to suggest a different approach here:
Using time.Ticker. The implementation would look like this:

ticker := time.NewTicker(time.Minute)
for {
	select {
	case <-ticker.C():
              // perform action
         case <-ctx.Done():
              ticker.Stop()
        }
}

(You should call the ticker.Stop() only if you want to reconcile the FS before exiting)

[EnriqueL8]

How does either of these solutions handle the situation where there is another case statement which in this case is waiting for events that the file has been renamed, written too etc... This also acts as a "sync" I think the current proposed code has an issue that timer is actually not reset in this case if we want it to be.

In the first approach, you would just cancel the context when handling this case and go around the loop again with the timer reset. In the second approach, there is a ticker.Reset(d duration) so both approaches work.

[onelapahead]

Thanks Enrique - yes it's really important we continue to sync the file over time in the case of Reconcile. The reason for these proposed alternatives is that the case <-time.After I've proposed is essentially optional in the Watch usecase. And so we'd occasionally do unnecessary CPU work when the timer pops, and go back around the loop to wait for the FS events, errors, or ctx to finish.

In a frequently changing FS, that could incur a performance hit. However, I don't believe this Watch is optimized for such rapid changes in the first place. A user would likely prefer to implement their own watcher in that case.

For the main case of a file which may changes on the scale of seconds or more, I don't think an extra case for this timeout would be detrimental, and I think the simplicity/readability of the code is preferred to a wrapper context/nested select statement which could have new bugs in it as Enrique pointed out we have to reset the loopCtx, etc.

So if its alright with the maintainers, I'd prefer to keep it as is.

[peterbroadhurst]

So if its alright with the maintainers, I'd prefer to keep it as is.

ok - so just marking where we're landing with this PR:

• This code will wake up every minute, for all existing installations. This is logically like a time.Ticker(), but we're using a time.After().
• The code will wake up and just go straight back to sleep if no reconciler is configured

All the comments are around code style opinions, and (as per my original comment) I'm not laying down on the tracks for one particular style. So happy to merge.