Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Task is not getting re-executed after addition/deletion of files when used with --watch argument. #1179

Open
PaulisMatrix opened this issue May 22, 2023 · 11 comments
Labels
area: watcher Changes related to the Taskfile watcher. type: bug Something not working as intended.

Comments

@PaulisMatrix
Copy link

PaulisMatrix commented May 22, 2023

Please describe the bug you're facing. Consider pasting example

Task is not getting re-executed after addition/deletion of files when used with --watch argument.
Below is the sample taskfile for it.

version: '3'

tasks:
  go-runs:
    desc: Task for building a new binary
    cmds:
      - go build -o main
      - go install 
      - echo 'new binary available'
    sources:
      - '**/*.go

Works fine without --watch arg when the command task go-runs is ran but have to do that manually every time which can be avoided by using --watch arg but its not working for it as explained above.

  • Task version: 3.0
  • Operating System: MacOS 12.4

It seems to be an issue from the watcher pkg itself.
radovskyb/watcher#120

@task-bot task-bot added the state: needs triage Waiting to be triaged by a maintainer. label May 22, 2023
@radcool
Copy link

radcool commented Sep 11, 2023

@PaulisMatrix, I've observed the same behavior as you. I also expected tasks being watched to execute anytime a file matching those tasks' sources' glob pattern would be added, but noticed that they weren't.

I took at the code of watch.go and realized this is what is happening:

  1. When task <task_name>... -w is run, for every task a goroutine is started to listen to watcher events.

  2. At the same time, a second go routine is started to scan for files matching the sources' glob pattern. Any files that match get added to the watcher. The goroutine sleeps the watchInterval duration, and then scans for files matching the sources' glob pattern all over again. Any file the watcher isn't already watching gets added to the watcher. This is how files created after the task program has started get added.

  3. The watcher watches any file added (whether initially or subsequently) and sends an event on a channel to the first goroutine every time one of the files changes (size, content, permissions, etc.). Whenever such a file change event is received, the first goroutine runs the task again.

  4. The watcher doesn't send any event info for a file it is not watching (obviously). But also, once a file is added to the watcher, the watcher takes note of the file's current state and only triggers an event for that file if the file changes subsequently. In other words, the operation of adding a file to the watcher, by itself, doesn't trigger an event. And since no watcher event gets triggered, the task doesn't get re-run when a new file is created.

Looking at the watcher library code, it seems the only way for watcher to generate an event for file creation is by watching the file's containing directory. When a directory is watched, files created and deleted within that directory generate a watcher event.

One way for task to match the expectation described in this issue would be to have it also watch the directory in which the task is run. This would generate watcher events for any file creation or deletion. However, even if the sources' glob pattern only matched a few files, watching the entire task directory recursively could potentially put a sizeable burden on the watcher process if the task directory were to contain a large number of files. Also, any file change in the directory, whether the file matched the glob pattern or not, would trigger an event, which would run re-run the task multiple times, potentially unnecessarily. The events would have to be filtered against the glob pattern to only have any actual effect if they matched. This could be more trouble than it's worth.

An alternative, which I tried and seems successful, is to make two modifications to the watcher code:

  1. Add a boolean variable to the second goroutine:

    task/watch.go

    Line 92 in 84ad005

    go func() {
    to keep track of whether or not this is the first time the file scan has run (upon task startup) or if this is a subsequent scan (to detect files added after task has been started).

  2. If this is a subsequent scan, after adding the newly-created file to the watcher in the "registerWatchedFiles" function:

    task/watch.go

    Line 122 in 84ad005

    func (e *Executor) registerWatchedFiles(w *watcher.Watcher, calls ...taskfile.Call) error {
    manually trigger a file creation event using the TriggerEvent() method, which would send an event to the first goroutine and execute a re-run of the task.

@andreynering, @pd93, what are you thoughts on this? Do you believe that the current behavior, as it stands, is the correct one, or do you believe it should be modified, perhaps with a change similar to my proposal? Here is a quick-and-dirty implementation of the change that seems to work:

diff --git a/watch.go b/watch.go
index e390d16c..8b0446cf 100644
--- a/watch.go
+++ b/watch.go
@@ -90,12 +90,18 @@ func (e *Executor) watchTasks(calls ...taskfile.Call) error {
        }()
 
        go func() {
-               // re-register every 5 seconds because we can have new files, but this process is expensive to run
+               // re-register every "watchInterval" duration because we can have new files, but this process is expensive to run
+
+               // need to differentiate between the first scan of the sources and any
+               // subsequent ones used to catch added files
+               initialScan := true
+
                for {
-                       if err := e.registerWatchedFiles(w, calls...); err != nil {
+                       if err := e.registerWatchedFiles(w, initialScan, calls...); err != nil {
                                e.Logger.Errf(logger.Red, "%v\n", err)
                        }
                        time.Sleep(watchInterval)
+                       initialScan = false
                }
        }()
 
@@ -119,7 +125,7 @@ func closeOnInterrupt(w *watcher.Watcher) {
        }()
 }
 
-func (e *Executor) registerWatchedFiles(w *watcher.Watcher, calls ...taskfile.Call) error {
+func (e *Executor) registerWatchedFiles(w *watcher.Watcher, initialScan bool, calls ...taskfile.Call) error {
        watchedFiles := w.WatchedFiles()
 
        var registerTaskFiles func(taskfile.Call) error
@@ -161,6 +167,13 @@ func (e *Executor) registerWatchedFiles(w *watcher.Watcher, calls ...taskfile.Ca
                                if err := w.Add(absFile); err != nil {
                                        return err
                                }
+                               if !initialScan {
+                                       fileInfo, err := os.Stat(absFile)
+                                       if err != nil {
+                                               return err
+                                       }
+                                       w.TriggerEvent(watcher.Create, fileInfo)
+                               }
                                e.Logger.VerboseOutf(logger.Green, "task: watching new file: %v\n", absFile)
                        }
                }

@andreynering andreynering added area: watcher Changes related to the Taskfile watcher. and removed state: needs triage Waiting to be triaged by a maintainer. labels Oct 7, 2023
@andreynering
Copy link
Member

Hi @radcool!

I think that ideally Task should be able to detect new files that matched the glob, yes.

Given you worked a little with the code, are you willing to open a PR with this change?

@andreynering andreynering added the type: bug Something not working as intended. label Oct 7, 2023
@leaanthony
Copy link
Contributor

Is this the approach you want to take or the fsnotify route? The only efficient way to do that on Mac is via cgo IIRC. fsnotify is ok but you can hit inode limits on macOS pretty quickly with it. Mac has a native way of monitoring files that's very efficient however it requires the use of CGO

@andreynering
Copy link
Member

@leaanthony We've been avoiding requiring CGO in the codebase, if possible.

It seems that fsnotify does not support recursive watch as well. It's in the roadmap since 2024: fsnotify/fsnotify#18.

So, it seems that keeping the current polling library is the best route for now. I'm open to suggestions, though.

@radcool
Copy link

radcool commented Oct 14, 2023

Given you worked a little with the code, are you willing to open a PR with this change?

Sure thing @andreynering, although I do have a silly question: I'd like to modify watch_test.go to unit test the added feature in the PR, but I'm not able to run go test watch_test.go, even before modifying it.

Instead, I get:

$ go test watch_test.go 
# command-line-arguments_test [command-line-arguments.test]
./watch_test.go:97:21: undefined: shouldIgnoreFile
FAIL	command-line-arguments [build failed]
FAIL

I haven't written much test in Go and am not familiar with the testing framework. What is the correct procedure I should use to run these unit tests?

@sirenkovladd
Copy link

@radcool, do you have any progress on this or do i have to do it from scratch?

@radcool
Copy link

radcool commented Feb 14, 2024

@sirenkovladd, I had written some candidate code, but was stuck at writing the test for it, which is why I posted #1179 (comment). I was waiting for a reply to it before proceeding.

@sirenkovladd
Copy link

Could you post the code you have now somewhere?

@radcool
Copy link

radcool commented Feb 15, 2024

@sirenkovladd, the candidate diff I had written is in #1179 (comment). I really was just waiting to have #1179 (comment) answered before I submitted it because I wanted the PR to include a proper test.

@pbitty
Copy link
Contributor

pbitty commented Sep 9, 2024

@radcool , re: #1179 (comment), it seems this was fixed in #1567.

You should be able to run the tests now.

@radcool
Copy link

radcool commented Sep 21, 2024

@radcool , re: #1179 (comment), it seems this was fixed in #1567.

You should be able to run the tests now.

Thanks @pbitty. I've written the test and submitted PR #1828.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: watcher Changes related to the Taskfile watcher. type: bug Something not working as intended.
Projects
None yet
Development

No branches or pull requests

7 participants