haskell-fswatch / hfsnotify

Unified Haskell interface for basic file system notifications
BSD 3-Clause "New" or "Revised" License
136 stars 40 forks source link

Naive debouncer #103

Open tau-mask opened 2 years ago

tau-mask commented 2 years ago

Hello,

This PR is an attempt at providing a simple debouncer that could be good enough for the project to move on.

As far as I can tell, any debouncer that tries to check for the files' presence on the filesystem is bound to fail eventually. By the time the Haskell library receives an event and tries to access the filesystem, one should assume that a number of other changes might have happened. This is why I removed the check from flushEvents.

I could have followed the state machine as described in #55 but adding more logic to start from an unknown state (since we shouldn't bother with the filesystem) and to cater for the recent event type additions like CloseWrite would have quickly gotten out of hand. I'll let smarter people come up with smarter debouncers.

To that end, I also opted for a Debouncer class, in the hope that other people can plug in their own ideas of debouncers. I do not know if the interface is good enough for reuse. I'm a Haskell beginner so it's hard to tell.

tau-mask commented 2 years ago

Thanks for the contribution @tau-mask ! This looks promising. Any chance you could provide some tests?

I've been looking at the tests already there and I think I can manage to write some for the debouncing logic. This might take me a few days.

I understand the rationale for not handling CloseWrite etc., but it's a little concerning. On Linux, CloseWrite is one of the most useful events because it tells you someone has just written a file and you can now process it. It looks like NaiveDebouncer will debounce a [Modified, CloseWrite] into just Modified, which is unfortunate. If we want to publish a debouncer that does this we'll have to communicate very clearly about these caveats.

Agreed, I was actually trying to come up with a small reloader logic for a web server and not having CloseWrite was what prompted me to look deeper into this project. I think that there's no "one size fits all" when it comes to debouncers, and that's why I made Debouncer a type class. So writing a CloseWriteDebouncer (totally made-on-the-spot name, I hope I can think of something better) could be a solution for waiting for files to have been updated, and it could be set as default.

Not trying to over-complicate your design, but I was wondering whether it could short-circuit when an event like CloseWrite occurs? I.e. just emit the event it's gathered so far, then emit CloseWrite, and then continue processing the remaining events. This might result in the client getting more events than expected, but might be more correct?

I think this is possible if I change the signature of combineEvents to a -> FilePath -> [Event] -> [Event], and some adjustments are made to flushEvent (assuming I correctly understood how makeDebouncedAction works). This looks like a better solution overall, I'm going to give it a try.

thomasjm commented 2 years ago

Btw, I was thinking we could also support ModifiedAttributes (and even future modification types) by adding bits to the StModified* constructors. Something like this:

data InternalState
  = ...
  | StModifiedFirst ModificationInfo
  | StModifiedThen ModificationInfo

data ModificationInfo = ModificationInfo { 
  modificationInfoContents :: Bool
  modificationInfoAttributes :: Bool
}

So these bits get flipped on in response to the corresponding event types, and at the of the day we emit an event for each on bit.

thomasjm commented 2 years ago

Hey @tau-mask, just checking in -- I'd love to get this merged, is it ready for another review?

tau-mask commented 2 years ago

Hi,

sorry I've been quite busy lately and had not much time to spare for this. I want to come up with a solid design, but I lack experience in this area so it's taking some time to mature.

tau-mask commented 2 years ago

Here is my attempt at writing a debouncer focused on file reloading. I can't help but feel that it is not quite ready yet. My main concern is about the CloseWrite event being available exclusively on Linux. Other OSes might see little benefit here and that might not be the right choice for a default debouncer. I also tried to take into account your remarks, but every time I try to do something more detailed, the complexity of the state machine increases too much for a mechanism that should remain simple.

Also, I do not have tests for the code, running the complete test suite takes several minutes on my machine and I couldn't figure out how to make the tests run in parallel (or how to focus on a subset of these).