Rescan flag is correctness hazard

[ijackson]

Almost every caller of notify needs to handle rescan events, to be correct.

However, in order to discover that this thing even exists it is necessary to dig many levels deep in the documentation. In particular, starting from Event, one must navigate the following:

• attrs: EventAttributes

Additional attributes of the event.

Arbitrary data may be added to this field, without restriction beyond the Sync and Clone properties. Some data added here is considered for comparing and hashing, but not all: at this writing this is Tracker, Flag, Info, and Source.

This does not look like somethilng one needs to consider in a simple program. Most readers will only look here if they are trying to find how to do something complicated and can't find the API for it elsewhere.

• pub fn flag(&self) -> Option<Flag>

Retrieves the Notify flag for an event directly, if present.

Again, this text does not even hint that looking at this flag is necessary for a reliable program.

• Enum notify::event::Flag

Special Notify flag on the event.

This attribute is used to flag certain kinds of events that Notify either marks or generates in particular ways.

Once again, the reader is discouraged from reading on. Eventually we come to this:

• Variants: Rescan

Rescan notices are emitted by some platforms (and may also be emitted by Notify itself). They indicate either a lapse in the events or a change in the filesystem such that events received so far can no longer be relied on to represent the state of the filesystem now.

An application that simply reacts to file changes may not care about this. An application that keeps an in-memory representation of the filesystem will need to care, and will need to refresh that representation directly from the filesystem.

Here, at least, the careful reader will notice that actually this flag means "we have missed stuff". But you have to read this text quite carefully. The critical information is in the last sentence. (I don't really agree that "An application that simply reacts to file changes may not care about this". Any application that doesn't also separately poll will malfunction if rescan lapses occur but or not handled.)

I think this API is setting up developers for failure. Specifically, it is setting up developers for writing programs which mysteriously lose file updates but only when the system is under load.

IMO Event should have a structure which makes it impossible to access paths or kind without checking for Rescan. After all, a Rescan event conceptually applies to all paths and all kinds, so an application which filters on paths or kind must know about Rescan.

(edit: FTAOD I am talking about the 5.x preview API here.)

[0xpr03]

(I took the liberty of formatting your comment to better distinguish the quotes, as I've had a hard time parsing it at first)

[0xpr03]

If possible we could move that Event towards its own EventKind, not sure about that.

And think about how we handle this for the debouncer (re-initialize?)

[0xpr03]

This could be a blocker for v5 if we change the types, I think I'd rather skip this and wait for v6 ? CC #249

[ijackson]

If possible we could move that Event towards its own EventKind, not sure about that.

Moving it to its own kind would help. That way people who filter on kind would have to decide what to do about it.

But I think you also want to do something about paths. A naive path-filtering approach would be event.paths.any(|p| ...) and that would DTWT for rescan events - discarding them. I guess you could make paths be Option<Vec<Paths>>, and change the docs to say "the paths, if known; None means possibly any path might be affected".

Sorry to report this so late in the 5.x process. I only came across it while reviewing an "update to notify 5.x" MR in a project that uses notify (Arti).

[0xpr03]

What does DTWT mean ? (Same for FTAOD)

[0xpr03]

Embarrassing fact: I've never had to interact with that Flag before. So I didn't actually remember it existed.. Yeah that API is bad.

[0xpr03]

But I think you also want to do something about Paths

I think you should never expect there to be a nonzero list of elements. So I don't see how that would be a problem.

Regarding use cases:
My stance is that you shouldn't rely on any specific Event. You should actually re-scan after some time. That's what syncthing does (for a good reason). And other systems just want to recompile-on-change, so they don't care about the exact event, in fact they only need a debounced info. Then there is the problem that network devices will misbehave, so Polling is actually the only reliable option there. But do you actually know when it is a network share ?

[trinity-1686a]

As an alternative, having some fn need_rescan(&self) -> bool {matches!(self.attrs.flag(), Some(Flag::Rescan))} on Event, with some documentation highlighting how important it is to actually check that, would already go a long way I believe.

[ijackson]

DTWT = Do The Wrong Thing (cf DTRT = Do The Right Thing). FTAOD = For The Avoidance Of Doubt.

[ijackson]

But I think you also want to do something about Paths

I think you should never expect there to be nonzero List of Elements. So I don't see how that would be a problem.

I'm not sure I follow. I haven't tested (or UTSL) but I believe the rescan event has an empty paths? ISTM[1] that event.paths.any(|p| is_interesting_for_us(p)) is a reasonable thing someone might write as a path filtering strategy. But it would erroneously discard all rescan events as "not being for any path we care about".

[1] It Seems To Me

[trinity-1686a]

Should I submit a PR with my "low effort solution", while a longer-term (6.x) solution is being discussed?

[0xpr03]

Yeah I'd appreciate that.