Adds basic file watcher #1013
Adds basic file watcher #1013
Conversation
# Conflicts: # frankenphp.go # go.mod # worker.go
|
Actually the test needs to create a file while running in the pipeline (to trigger the file watcher), but it looks like it's not allowed to do so due to permissions. |
|
Hi @AlliBalliBaba, thank you very much for working on this feature! This is a must-have. I also started the work on my side and created https://github.com/dunglas/go-fswatch for this purpose some weeks ago. My bindings rely on cgo, but as we need cgo anyway, I don't think that's an issue. The benefits of the C/C++ Would you consider switching to this library? |
|
Hey yeah, I'll try swapping to |
|
I switched to your watcher and it works great 👍
|
|
Awesome!! In addition to We'll also avec to modify I can do this part when I'll have some time (likely after the API Platform Conference) if you want. Regarding wildcards (globs?), I don't think that it's natively supported, but regexp are more powerful anyway. My idea was to let users define custom regexp, but defaults to a regexp that will work for standard installations of Laravel and Symfony (and maybe other popular projects such as WordPress and Drupal) so most users will not have to figure out the regexp (or the glob) by themselves. WDYT about this approach? |
|
So I tried installing During testing I also noticed that there will sometimes be a segfault with low watcher latencies (<100ms) expandgo test SIGSEGV: segmentation violation PC=0x7f2666817f9b m=5 sigcode=2 addr=0x7f263c000ef4 signal arrived during cgo executiongoroutine 7103 gp=0xc000510c40 m=5 mp=0xc000100008 [syscall]: goroutine 1 gp=0xc0000061c0 m=nil [chan receive]: goroutine 2 gp=0xc000006c40 m=nil [force gc (idle)]: goroutine 3 gp=0xc000007180 m=nil [GC sweep wait]: goroutine 4 gp=0xc000007340 m=nil [GC scavenge wait]: goroutine 5 gp=0xc000007c00 m=nil [finalizer wait]: goroutine 6 gp=0xc0001861c0 m=nil [sync.Cond.Wait]: goroutine 220 gp=0xc000186a80 m=nil [GC worker (idle)]: goroutine 273 gp=0xc000187dc0 m=nil [GC worker (idle)]: goroutine 271 gp=0xc0001c2000 m=nil [GC worker (idle)]: goroutine 272 gp=0xc0001c21c0 m=nil [GC worker (idle)]: goroutine 270 gp=0xc0001c2380 m=nil [GC worker (idle)]: goroutine 268 gp=0xc0001c2540 m=nil [GC worker (idle)]: goroutine 267 gp=0xc0001c2700 m=nil [GC worker (idle)]: goroutine 219 gp=0xc0001c2fc0 m=nil [GC worker (idle)]: goroutine 221 gp=0xc0001c3340 m=nil [GC worker (idle)]: goroutine 269 gp=0xc0001cea80 m=nil [GC worker (idle)]: goroutine 218 gp=0xc0001cf340 m=nil [GC worker (idle)]: goroutine 223 gp=0xc0001cfc00 m=nil [GC worker (idle)]: goroutine 235 gp=0xc0001d88c0 m=nil [GC worker (idle)]: goroutine 254 gp=0xc0001d8a80 m=nil [GC worker (idle)]: goroutine 252 gp=0xc0001d8c40 m=nil [GC worker (idle)]: goroutine 255 gp=0xc0001d9180 m=nil [GC worker (idle)]: goroutine 251 gp=0xc0001d9a40 m=nil [GC worker (idle)]: goroutine 253 gp=0xc0001e2c40 m=nil [GC worker (idle)]: goroutine 222 gp=0xc0001ece00 m=nil [GC worker (idle)]: goroutine 224 gp=0xc0001ed500 m=nil [GC worker (idle)]: goroutine 132 gp=0xc0004028c0 m=nil [select, locked to thread]: goroutine 190 gp=0xc000273500 m=nil [select, locked to thread]: goroutine 198 gp=0xc00064cfc0 m=nil [select, locked to thread]: goroutine 199 gp=0xc00064d6c0 m=nil [select, locked to thread]: goroutine 203 gp=0xc00067f340 m=nil [select, locked to thread]: goroutine 206 gp=0xc000760700 m=nil [select, locked to thread]: goroutine 208 gp=0xc000511180 m=nil [select, locked to thread]: goroutine 211 gp=0xc000562700 m=nil [select, locked to thread]: goroutine 212 gp=0xc000562e00 m=nil [select, locked to thread]: goroutine 213 gp=0xc000563500 m=nil [select, locked to thread]: goroutine 214 gp=0xc000563c00 m=nil [select, locked to thread]: goroutine 7104 gp=0xc000403dc0 m=12 mp=0xc000580008 [syscall]: goroutine 7368 gp=0xc00042ca80 m=nil [semacquire]: goroutine 7369 gp=0xc000129dc0 m=nil [chan receive]: goroutine 7105 gp=0xc0009be380 m=nil [runnable]: rax 0x4 Additionally I can't modify the github workflows and fswatch would have to be installed everywhere there as well So I think it's better if you do it. The dependency on |
|
I wonder if it's possible to bundle up the fswatch binary into go-fswatch directly so we could skip all these build steps. |
|
And to adress your regex question, my original intention was to stay compatible with the wilcard syntax that chokidar also uses, like here in laravel octane: But I'm also fine with regex if there are other ways to abstract that away, people just tend to shoot themselves in the foot with regex 😅 |
|
Bundling the C lib in go-fswatch would be practical but will require huge CI work (similar to what we've done here for FrankenPHP) because we'll have to cross-compile the C lib. Also, this is likely not what many users want as libfswatch is packaged by some distributions, including Debian (unfortunately, the current packaged version is too old to be used by go-fswatch, but it will likely be updated at some point). IMHO it would be easier to handle that part directly in FrankenPHP, as we have most of the needed infrastructure. Regarding regexp, I wouldn't bother with Chokidar compatibility. Chokidar will continue to work in Octane (so existing setups will not be broken). I agree that regexp are harder to master, but they are also more powerful. |
|
Alright we can go with |
…the {bracket} pattern.
|
I guess we'll wait for #1038 to merge first? There will probably be conflicts |
|
I did it with your changes in mind, so they should be pretty straightforward if you merge. Rebasing (I didn't take into account your change history) might be far more complicated. |
|
@AlliBalliBaba do you mind if I rebase and merge this one? (I would like to test it on Mac) |
|
Sounds good to me 👍 |
|
I'm not sure of the interaction of |
|
I dislike the Waitgoup.Add(1) here, that's why I removed it. Its only purpose is to not have the waitgroup go negative since we don't care about it anymore after startup anyways. |
|
I also plan to make a PR that stores a slice of |
|
Good idea for the thread slice! Finally, would you mind merging main in your branch (or rebasing)? I'" not sure to be able to do it without introducing mistakes. |
# Conflicts: # worker.go
|
Do you mind if I go for the rebase? |
|
@withinboredom I just merged it 😅, read your message a few minutes too late. I adjusted the backoff constants a bit, since the server shouldn't crash immediately when editing the worker file itself, here's a video showing what I mean: Do you think 1sec maxBackoff + 60 retries is fine @withinboredom ? Also I noticed that tests got a lot slower after merging 6s -> 15s. Not sure where that comes from. |
|
@withinboredom what do you think as merged this patch as soon as you reviewed it, so we can iterate (if needed) in follow-up PRs? |
It's from |
|
Yeah, lets merge this @dunglas and any issues can be handled in PRs. LGTM |
|
Thank you very much @AlliBalliBaba. Huge work! |
|
FYI, fsnotify has some initial support for recursion. It's only for Linux right now, and it isn't enabled yet. But the dev gave me some instructions here on how to enable it. Not sure if that could be helpful to simplify things here/reduce tech debt. |
|
@nickchomey thank you for the info! We definitely plan to switch to a pure go solution at some point since it will make compilation easier. |
This PR adds a basic version of a file watcher to reload workers when changing files. The watcher is based on
github.com/fsnotify/fsnotifysince it's already a downstream dependency of the caddfy module.Some notes:
filepath.Walkwatcher_test.gofail, which I don't yet quite understand (it's related to theworkersReadyWG)