Add file watcher API and Linux implementation

[meyraud705]

Description

Add 1 public function: bool SDL_WatchFileForChanges(const char *path) and 2 event types: SDL_EVENT_FILE_WATCH_ERROR and SDL_EVENT_FILE_CHANGED.

SDL_UpdateFileWatch() is called when pumping event to update file watch. Resources are cleared in SDL_QuitFilesystem(void).

Implement this API for Linux using inotify. I put it in SDL_sysfsops.c, I don't know how other platforms work so that may not be the correct place.

Existing Issue(s)

Implement #15070 for Linux.

[Semphriss]

A design consideration: would it be possible to use a callback instead of an event? If a given program listens to different paths at different places in the code, events can make it difficult to transfer the signal from the event loop down to the specific part of the code that needs that event. That was my reasoning when developing the file dialog subsystem; using events would've split the part of the code that invokes the dialog from the part that handles the subsequent file operations, which can make a repo quite messy if not carefully managed.

[slouken]

A design consideration: would it be possible to use a callback instead of an event? If a given program listens to different paths at different places in the code, events can make it difficult to transfer the signal from the event loop down to the specific part of the code that needs that event. That was my reasoning when developing the file dialog subsystem; using events would've split the part of the code that invokes the dialog from the part that handles the subsequent file operations, which can make a repo quite messy if not carefully managed.

I'd also like to see this be callback based rather than event based. You have more local control, and it's always possible to generate an event from a callback.

@icculus, thoughts?

[icculus]

Some counter-arguments here:

• if you do a callback, then you have threading questions to deal with. You're probably going to end up synchronizing file changes on another thread until around the time you are pumping the event loop anyhow.
• "hey this file changed" is maybe something you do want to deal with once a frame, possibly with several updates collapsed into a single event.
• I'm not certain that all platforms will give this to you immediately, so a callback might not offer instant-response in all cases.

But, ultimately:

• events are useless if you don't use the event subsystem, but even in a command line tool with no window, waiting for file changes could be useful.

[meyraud705]

You have more local control,

With event, you only need to make your callback public and have a dispatch function in the event loop. Doesn't look messy to me and you know when and from which thread you reload the file.

SDL_Event e;
while (SDL_PollEvent(&e)) {
    if (e.type == SDL_EVENT_FILE_CHANGED) {
        if (SDL_strncmp(e->file_watch.path, "./shader/", 9) == 0) {
            ReloadShader(&e->file_watch.path[9]);
        } else if (...) {
            ...
        }
    }
}

and it's always possible to generate an event from a callback.

It is also possible to generate a callback from an event with SDL_AddEventWatch() (callback needs to check if it cares about the file path though).

[Semphriss]

For simple enough use cases, I don't expect events to be too complicated to manage, but for more advanced uses, I see a number of problems arising:

• Handling path changes requires passing every watch through the event loop. This isn't much of a problem when the file watching logic plays with global information, but anything deeply nested in the application logic would need to expose a function and rely on the main loop to invoke it, which creates tight coupling between the various components in the code.
• If the use case is too complex for the path to be watched to be described by a string literal (for example, in case the path to be watched can be dynamically specified), the if/else block has to keep track of every registered path to be watched and know which callback(s) to invoke based on it.
• As stated, the downside with SDL_AddEventWatch is that every callback is invoked on every file change. This requires checking the file path in each callback, which in turn requires keeping track of the watched paths for each callback if they're not constant. I'm not sure what would be the performance impact as the total number of watched paths and corresponding callbacks increase.
• Making object wrappers for file watchers become more difficult to implement, since they have to be bound to the event loop in some way.

IMO, it would be easier to emit an event from within a callback than to simulate a callback from the event loop, since it requires less refactoring. With callbacks, the code requires only to add one function in the same file where the watching is registered:

void callback(const char* path)
{
  SDL_Event event;
  SDL_zero(event);
  event.file_watch.type = SDL_EVENT_FILE_CHANGED;
  event.file_watch.timestamp = SDL_GetTicksNS();
  event.file_watch.path = path;
  SDL_PushEvent(&event);
}

void fn()
{
  // ...

  SDL_WatchFileForChanges("/path/to/file", callback);

  // ...
}

Conversely, wiring callbacks from events will require keeping track of path-callback pairs, sending them to the main loop, and adding logic in the main loop to find which callback to invoke when a path notification arises. It's feasible, just more complicated.

[meyraud705]

You convinced me, I'll make another PR with callback.

[slouken]

You convinced me, I'll make another PR with callback.

There's no reason we can't do both, where the callback is optional and we always send a file changed event.

[slouken]

Typically userdata will be the first argument to the callback.

[slouken]

It would also be helpful to know the thread safety situation for this callback. (Is it guaranteed to be called on a specific thread, etc.)

[meyraud705]

It run on its own thread, but I don't know how other platforms will handle this so I documented it can run on any thread.

[slouken]

Suggest flipping this, so the normal change event comes first.

[meyraud705]

Done.

[slouken]

Does it make sense to add a separate API for watching an entire directory?

[slouken]

Or if this function handles both, would it make sense to be called WatchPathForChanges?

[meyraud705]

inotify handles both so does this function. You only get event when a file is modified though.

[slouken]

This looks good to me, @icculus, thoughts?

[madebr]

Is it possible to remove a file watcher?

[slouken]

Oh yes, this should be AddPathWatch() with a corresponding RemovePathWatch()

[madebr]

Should this remove all trailing slashes?

Suggested change

	if (watch_entry->path[slen - 1] == '/') {
	watch_entry->path[slen - 1] = '\0';
	}
	while (slen > 0 && watch_entry->path[slen - 1] == '/') {
	watch_entry->path[--slen] = '\0';
	}

[meyraud705]

No, only last slash is removed because it is added back when concatenating directory path and file name.

The reason for not removing all trailing slashes is that the path your receive in event is the same as the one you pass to SDL_WatchPathForChanges(), so that you can strncmp them.

const char *path = "/path/.///";
SDL_WatchPathForChanges(path, NULL, NULL) ;
/* in event loop: */
if (SDL_strncmp(event.file_watch.path, path, SDL_strlen(path))) {
    const char *file_name = &event.file_watch.path[SDL_strlen(path)];
    ...
}

Maybe we should add SDL_CanonicalizePath() or SDL_ComparePath().

[madebr]

Suggested change

	static int SDL_FileWatchThread(void *user_data);
	static int SDLCALL SDL_FileWatchThread(void *user_data);

[madebr]

Can/should this feature have a test?

[madebr]

This also needs a test for slen > 0.
This also begs the question: is adding a file watcher for the path "" valid?

[slouken]

No, we should check and reject "" as a path.

[madebr]

What about SDL_EVENT_FILE_WATCH_OVERFLOW?
Or do file watchers have multiple error conditions?

[meyraud705]

inotify uses a queue which can overflow. I don't know error for other platform so I kept he name generic.

[madebr]

the callback will be called for every file modified in that directory

Does this include the directory? Or only its entries?

[madebr]

What about file creation and deletion? I suppose we also get notified about those.

[meyraud705]

Modify means a call to write() on the file. You cannot write to directory. If the documentation is not clear please propose another wording.

Directory watch are not recursive.

There is no notification for file and directory creation, deletion or rename. I could add these if you want them.

[icculus]

This looks good to me, @icculus, thoughts?

I'm fine with this once @madebr's feedback is resolved.

(And obviously don't cherry-pick this to 3.4.x, this is clearly 3.6.0 work.)

[slouken]

This looks good to me, @icculus, thoughts?

I'm fine with this once @madebr's feedback is resolved.

(And obviously don't cherry-pick this to 3.4.x, this is clearly 3.6.0 work.)

Yup. :)

[madebr]

If this event includes creation and deletion, I'm not sure the name (and doc string) covers them all.

[madebr]

I don't see a check that read had read a complete event. Can't this access uninitialized or stale memory?

[meyraud705]

Read of inotify event is copied from https://github.com/libsdl-org/SDL/blob/main/src/joystick/linux/SDL_sysjoystick.c#L764

[madebr]

Is polled reading used for a reason? Would blocked I/O work?

[meyraud705]

We could use poll() or select() with a timeout if you think it is better.

[madebr]

I don't know either. I'm just asking questions :)
The joystick inotify code runs on the main thread so cannot block, which is not the case here.

[madebr]

Is select needed? We're just waiting on one file, not multiple. I'm not sure what to do about destruction and writing/reading to/from the descriptor in multiple threads.

[meyraud705]

It worked like the joystick system at the beginning because there was only event. But now there is callback so it needs to work even if you don't use the event subsystem.

You need a timeout to check if we should quit the thread.

All operations on descriptor are protected by a mutex.

[TerensTare]

Can we also get an equivalent API for directories? Reasons:

1. This makes the API easier to use in the common case (ie. one watcher for everything in assets vs one watcher for every image/shader in the directory.
2. AFAIK, ReadDirectoryChangesW (Windows) and FSEvent (macOS) work on directories by default (even inotify supports this), then you "filter" your files of interest.
3. The "watch file" API can then be written with the "watch directory" API, as following:

// rough sketch
void watch_file(const char *path, Callback cb) {
    const char *dir = GetFolder(path); // get the folder where this file is
    SDL_Hashtable *dir_table = INTERNAL_WatchDirAndGetTable(dir); // keep a table of (file -> callback) internally, per folder?
                                             // ^ This just returns the corresponding table if this directory was already registered
    Table-Insert(dir_table,  FileHandle(path), cb); // add callback to our table, to be called later
    // ^ actually this can be an array too, how many files can you possibly register individually within the same directory?
}