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

correct way to determine when file writes are finished #553

Closed
kmulvey opened this issue Jan 31, 2023 · 4 comments
Closed

correct way to determine when file writes are finished #553

kmulvey opened this issue Jan 31, 2023 · 4 comments

Comments

@kmulvey
Copy link

kmulvey commented Jan 31, 2023

What is the correct way to determine when file writes are finished in lieu of IN_CLOSE_WRITE? Currently, when watching a directory where large images are being written I receive a CREATE event followed by many (hundreds) of WRITE events. I am currently debouncing the write events to see if some amount of time has passed since the last WRITE to "call it good" and then begin reading the file. This approach works (kinda) but is not robust, I have to keep increasing the "time since last write" for larger images. Note on image sizes: we are only talking about 100M at the largest, so not anything crazy.

func debounceWrites(eventsIn, eventsOut chan path.WatchEvent) {

	var cache = make(map[string]TimeOfEntry)
	var ticker = time.NewTicker(200 * time.Millisecond)

	for {
		select {
		case event, open := <-eventsIn:
			if !open {
				close(eventsOut)
				return
			}

			cache[event.Entry.AbsolutePath] = TimeOfEntry{WatchEvent: event, Time: time.Now()}

		case <-ticker.C:

			for filename, entry := range cache {
				if time.Since(entry.Time) > 3*time.Second { // this is a hack and keeps getting increased 
					eventsOut <- entry.WatchEvent
					delete(cache, filename)
				}
			}
		}
	}
}
@arp242
Copy link
Member

arp242 commented Feb 1, 2023

There isn't really a good way to do this 100% reliably at the moment. The best approach is to wait a short period of time for more events. I don't really follow your code exactly because it's incomplete (and doesn't interact with fsnotify directly), but there's an example over here: https://github.com/fsnotify/fsnotify/blob/main/cmd/fsnotify/dedup.go

@jeromer
Copy link

jeromer commented Jun 23, 2023

If that was feasible in a correct way, would you accept a patch which add support for inotify's close events event if it breaks portability ? For what it's worth, fswatch does not support inotify's close events either for portability reasons as described in emcrisostomo/fswatch#212

@arp242
Copy link
Member

arp242 commented Jun 23, 2023

Adding support for these events a such is easy, even trivial @jeromer; The tricky bit is figuring out a good API for this.

fsnotify is a cross-playform library; the default should be as homogeneous as we can make it: if it works on e.g. Linux then it should work as identical as we can make it on other systems such as Windows.

That said, sometimes you just care about running something on one or two platform and you just don't care about others, but you should explicit have to opt-in to that by saying "I only care about Linux systems and I'm okay breaking support for others".

Actually, it's a little bit more complex than that, because if I remember correctly some kqueue platforms also support close event, so it's more "Linux and some but not all BSD systems".

So, with that in mind:

  • This issue is essentially "unsolvable", as far as I know, because not all platforms expose good enough primitives for it.
  • But we can opt-in to better behaviour on some platforms, we just need to figure out an API for it.

In #521 I added AddWith() which allows passing options. In my example (not implemented) I have:

err = w.AddWith("/path", fsnotify.WithEvents(fsnotify.Open | fsnotify.Close))

The Close event would document which systems it would support.

Is that explicit enough that you're dropping support for some platforms? I'm not 100% sure? Maybe this would be better:

err = w.AddWith("/path", fsnotify.WithUnportableEvents(fsnotify.Open | fsnotify.Close))

But it's quite long.

Also another somewhat feature that's been requested is filtering of events (e.g. "I only want Create events and nothing else"), so we should consider that too. Not necessarily support that, just make sure we have a plan and don't come to regret out API choices: do we want to have some fsnotify.With... for that or two?

One option is that WithEvents ONLY sends events that are listed, e.g. this would only do Create and Close:

err = w.AddWith("/path", fsnotify.WithEvents(fsnotify.Create | fsnotify.Close))

Another is to have two:

err = w.AddWith("/path",
    fsnotify.WithEvents(fsnotify.Open | fsnotify.Close), // Or WithUnportableEvents
    fsnotify.WithFilter(fsnotify.Create))

But would WithFilter() override WithEvents()? Seems a bit unclear to me 🤔

Also should referencing fsnotiy.Close be a compile error on unsupported platforms? Or just be ignored?


I'm not 100% sure about all of this.

Let me know what you think of this.

But in general: sure, I'm more than happy to accept patches. I think we should implement this, we just need to figure out all of the above.

@arp242
Copy link
Member

arp242 commented Oct 18, 2023

This will be documented in the next version, and allowing non-portable events is #67 and #519, so I think this can be closed.

@arp242 arp242 closed this as completed Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants