gonic icon indicating copy to clipboard operation
gonic copied to clipboard

Implementation of fsnotify based scan watcher

Open brian-doherty opened this issue 2 years ago • 10 comments

Tested on Linux.

brian-doherty avatar Jul 23 '22 21:07 brian-doherty

thanks Brian!! this looks nice . hoping to have a closer look tomorrow

sentriz avatar Aug 13 '22 23:08 sentriz

also interestingly, the likes of jellyfin and plex do filewatching too. i wonder do they have some trick up their sleeve to avoid the limits

sentriz avatar Sep 11 '22 13:09 sentriz

Re: watch limits -- if someone with more directories starts the watcher they should see errors in the log but it will watch what it can. Then if they have root access they can increase the system limit.

brian-doherty avatar Sep 12 '22 15:09 brian-doherty

just ran again locally and seems to work quite nice :+1:

i have some questions also

  • when starting gonic for the first time, it seems to do a scan of the root dir straight away. it it meant to do that? could be annoying for users with big libraries. and it feels like it shouldn't since there wasn't any explicit event from the kernel
  • are delete events supported? i did an rm * in my root dir and it didn't seem to pick it up. that's fine though i think. just wondering
  • perhaps we should debounce the events. eg if i have a folder outside my library like
    artistname/
    artistname/albumname/
    artistname/albumname/track1.flac
    artistname/albumname/track2.flac
    
    and copy that into my gonic root dir like
    $ cp -r artistname gonicroot/
    
    it picks up the artistname event straight away but never finds the album or tracks, since we haven't copied at the time of the first artistname/ event. have to do a manual scan. (at least for me in my testing)
  • also related to the deboucing, if we do
    for track in somefiles/*.mp3; do
        cp "$track" gonicroot/someartist/some/album/
    end
    
    (which simulates some slower copies - maybe over a remote filesystem or whatever) we doing a lot of scans. which is maybe fine - but if one scan takes longer than it does to copy to files, we miss the second event. i think a debounce helps us here too

what do you think? IIRC syncthing for example does some debounce stuff

sentriz avatar Sep 13 '22 18:09 sentriz

another QQ, could s.watchMap be replaced by s.watcher.WatchList() ?

also regarding the debounce, if you think it's a good idea, hugo do something similar here https://github.com/gohugoio/hugo/blob/master/watcher/batcher.go

(found from https://github.com/fsnotify/fsnotify/issues/401 )

sentriz avatar Sep 13 '22 18:09 sentriz

Re: initial scan -- yes, that was deliberate to make sure the library was up to date as we set the watches, but I'm fine with omitting it if that's what you want. Re: deletion -- I explicitly did not handle deletion as I figured it would get handled in the periodic scan just fine. Re: whole tree copy in -- that should work. I will have a look. Are you sure you didn't hit your max watch limit? Because in that case you'd see exactly the symp[tom you report. Re: watchMap -- we need the mapping back to which music dir root each directory is in in order to pass to the scanCallback

brian-doherty avatar Sep 13 '22 18:09 brian-doherty

Re: initial scan -- yes, that was deliberate to make sure the library was up to date as we set the watches, but I'm fine with omitting it if that's what you want.

yeah personally i think it would be good to not scan on startup, if that doesn't break anything

Re: deletion -- I explicitly did not handle deletion as I figured it would get handled in the periodic scan just fine.

:+1:

Re: whole tree copy in -- that should work. I will have a look. Are you sure you didn't hit your max watch limit? Because in that case you'd see exactly the symp[tom you report.

thanks, yes this is just testing on my laptop with just a few dirs

Re: watchMap -- we need the mapping back to which music dir root each directory is in in order to pass to the scanCallback

:+1:

sentriz avatar Sep 13 '22 19:09 sentriz

ah now it has started working for the whole tree copy. though it produces a lot of event. i think if we debounce/batch (maybe something like the hugo example?) it help a lot. any maybe make sure we don't miss any files

for example: ("watching" message turned off)

https://user-images.githubusercontent.com/6832539/189992502-9cee5b5d-5312-4e90-bb2d-03ad6c93895e.mp4

sentriz avatar Sep 13 '22 19:09 sentriz

We could go a couple of routes on the batching:

  1. Batch them all and just do a full scan when there's a change. Probably pretty wasteful.
  2. Batch and eliminate dup directories.
  3. Not bother batching and maybe reduce the logging on this thing so that it's not so noticeable.

I prefer 3 because of sloth :D but I'd be willing to take a stab at 2.

brian-doherty avatar Sep 13 '22 21:09 brian-doherty

personally like 2

not only seems like a good thing but I think solves the problem I initially had when I first tested (whole tree copied, notified only about first event, no children to add watchers to yet, meaning no children found (though I could be way off on this))

wdyt?

very happy to help out on this if your time is limited

sentriz avatar Sep 15 '22 18:09 sentriz

Let me see if I can get to it this week.

brian-doherty avatar Sep 19 '22 18:09 brian-doherty

Ok check it out.

brian-doherty avatar Sep 24 '22 22:09 brian-doherty

thanks brian! did some testing and seem to work very well. i just make scanList a map (seems like we can just incase we get duplicate events, then we don't try process them twice. does this make sense?) and removed the "watching folder" log message, since it's a lot with a big library

is this ok?

sentriz avatar Oct 08 '22 13:10 sentriz

OK by me. Thanks!

brian-doherty avatar Oct 08 '22 16:10 brian-doherty