diff --git a/NEWS.md b/NEWS.md index 62b0dca..7ad1829 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,7 @@ # watcher (development version) +* Paths reported by a single callback are now always unique, with duplicate filesystem events for the same path consolidated (#55, thanks @tomjemmett). + # watcher 0.2.1 * Fixes installation failing on systems with a partial or broken system 'libfswatch', now falling back to the bundled build (#48). diff --git a/R/watch.R b/R/watch.R index 2b328e3..7f23890 100644 --- a/R/watch.R +++ b/R/watch.R @@ -11,7 +11,8 @@ #' does not mean that changes are polled for at this interval. The changes are #' monitored in an event-driven fashion by the platform-specific monitor. Events #' are 'bubbled' such that a single change that triggers multiple filesystem -#' events will cause the callback to be called only once. +#' events will cause the callback to be called only once, and the paths +#' reported by a single callback are always unique. #' #' It is possible to set a watch on a path that does not currently exist, and it #' will be monitored once created. diff --git a/man/watcher.Rd b/man/watcher.Rd index dbcb923..9e1a9e3 100644 --- a/man/watcher.Rd +++ b/man/watcher.Rd @@ -34,7 +34,8 @@ Note: the \code{latency} setting controls how often the changes are processed, a does not mean that changes are polled for at this interval. The changes are monitored in an event-driven fashion by the platform-specific monitor. Events are 'bubbled' such that a single change that triggers multiple filesystem -events will cause the callback to be called only once. +events will cause the callback to be called only once, and the paths +reported by a single callback are always unique. It is possible to set a watch on a path that does not currently exist, and it will be monitored once created. diff --git a/src/fswatch/libfswatch/src/libfswatch/c++/monitor.hpp b/src/fswatch/libfswatch/src/libfswatch/c++/monitor.hpp index 564dea9..2a328ed 100644 --- a/src/fswatch/libfswatch/src/libfswatch/c++/monitor.hpp +++ b/src/fswatch/libfswatch/src/libfswatch/c++/monitor.hpp @@ -652,7 +652,7 @@ namespace fsw * @brief Bubble events by joining flags received for the same (time, path) * pair. */ - bool bubble_events = false; + bool bubble_events = true; /** * @brief Mutex used to serialize access to the monitor state from multiple diff --git a/src/watcher.c b/src/watcher.c index 61379ec..80e8886 100644 --- a/src/watcher.c +++ b/src/watcher.c @@ -23,12 +23,7 @@ static void watcher_error(FSW_HANDLE handle, const char *msg) { static void watcher_unwind(watcher_cb *wcb) { - if (wcb->paths) { - for (unsigned int i = 0; i < wcb->event_num; i++) { - free(wcb->paths[i]); - } - free(wcb->paths); - } + free(wcb->paths); free(wcb); } @@ -53,38 +48,50 @@ static void exec_later(void *data) { static void process_events(fsw_cevent const *const events, const unsigned int event_num, void *data) { SEXP callback = (SEXP) data; - watcher_cb *wcb = NULL; + + /* Single allocation: the pointer array, followed by the string storage. */ + size_t bytes = sizeof(char *) * event_num; + for (unsigned int i = 0; i < event_num; i++) { + bytes += strlen(events[i].path) + 1; + } + + watcher_cb *wcb = malloc(sizeof(watcher_cb)); + if (wcb == NULL) return; + wcb->event_num = 0; + wcb->callback = callback; + wcb->paths = malloc(bytes); + if (wcb->paths == NULL) { + watcher_unwind(wcb); + return; + } + char *store = (char *) (wcb->paths + event_num); + + /* Keep the first occurrence of each path: duplicates carry no information + * as only paths are surfaced to R (r-lib/watcher#55). */ + for (unsigned int i = 0; i < event_num; i++) { + unsigned int j = 0; + while (j < wcb->event_num && strcmp(events[i].path, wcb->paths[j])) j++; + if (j < wcb->event_num) continue; + wcb->paths[wcb->event_num] = store; + const size_t slen = strlen(events[i].path) + 1; + memcpy(store, events[i].path, slen); + store += slen; + wcb->event_num++; + } if (callback != R_NilValue) { - wcb = malloc(sizeof(watcher_cb)); - if (wcb == NULL) goto fail; - - wcb->event_num = event_num; - wcb->callback = callback; - wcb->paths = calloc(event_num, sizeof(char *)); - if (wcb->paths == NULL) goto fail; - for (unsigned int i = 0; i < event_num; i++) { - size_t slen = strlen(events[i].path) + 1; - wcb->paths[i] = malloc(sizeof(char) * slen); - if (wcb->paths[i] == NULL) goto fail; - memcpy(wcb->paths[i], events[i].path, slen); - } eln2(exec_later, wcb, 0, 0); } else { - for (unsigned int i = 0; i < event_num; i++) { - Wprintf("%d: %s\n", i, events[i].path); + for (unsigned int i = 0; i < wcb->event_num; i++) { + Wprintf("%d: %s\n", i, wcb->paths[i]); } + watcher_unwind(wcb); } - return; - - fail: - watcher_unwind(wcb); - } static void* watcher_thread(void *args) { @@ -108,7 +115,6 @@ static void session_finalizer(SEXP xptr) { SEXP watcher_create(SEXP path, SEXP callback, SEXP latency) { - const char *watch_path = Rf_translateChar(STRING_ELT(path, 0)); const double lat = Rf_asReal(latency); FSW_HANDLE handle = fsw_init_session(system_default_monitor_type); @@ -118,12 +124,8 @@ SEXP watcher_create(SEXP path, SEXP callback, SEXP latency) { /* no need to test return value of fsw_add_path() as it only errors if the * path is a null pointer, and Rf_translateChar() cannot return one */ - fsw_add_path(handle, watch_path); - - if (XLENGTH(path) > 1) { - for (R_xlen_t i = 1; i < XLENGTH(path); i++) { - fsw_add_path(handle, Rf_translateChar(STRING_ELT(path, i))); - } + for (R_xlen_t i = 0; i < XLENGTH(path); i++) { + fsw_add_path(handle, Rf_translateChar(STRING_ELT(path, i))); } if (fsw_set_latency(handle, lat) != FSW_OK) @@ -141,9 +143,9 @@ SEXP watcher_create(SEXP path, SEXP callback, SEXP latency) { * This is to prevent events being triggered too often as some platforms e.g. * Ubuntu generate events even when files are being read etc. */ - fsw_event_type_filter filter; - for (int flag = Created; flag <= Renamed; flag = flag << 1) { - filter.flag = flag; + static const enum fsw_event_flag flags[] = {Created, Updated, Removed, Renamed}; + for (unsigned int i = 0; i < sizeof flags / sizeof(flags[0]); i++) { + const fsw_event_type_filter filter = {flags[i]}; fsw_add_event_type_filter(handle, filter); } diff --git a/src/watcher.h b/src/watcher.h index dd72c3f..3cdf680 100644 --- a/src/watcher.h +++ b/src/watcher.h @@ -3,6 +3,7 @@ #include #include +#include #include #include #include diff --git a/tools/patch_libfswatch.sh b/tools/patch_libfswatch.sh index cfe3f45..71324ea 100755 --- a/tools/patch_libfswatch.sh +++ b/tools/patch_libfswatch.sh @@ -20,6 +20,11 @@ # watcher never enables libfswatch verbose logging, so this is a no-op in # normal use. Neutering removes the printf/stdout/stderr/cerr symbols that # would otherwise be flagged when libfswatch is compiled into watcher.so. +# 4. Bubble events by default - flip the monitor's bubble_events default to +# true. watcher surfaces only event paths to R, so duplicate same-path +# events within one latency window carry no information (r-lib/watcher#55). +# The C API does not expose set_bubble_events(); the vendored library is +# compiled solely for watcher, so flipping the default is safe. set -e @@ -138,3 +143,21 @@ if grep -q 'fprintf(stderr, _("Invalid handle when opening' "$WIN_MON" 2>/dev/nu else echo -e "${GREEN} ✓ windows_monitor.cpp fprintf(stderr) already removed${NC}" fi + +# --- Patch 4: enable event bubbling by default -------------------------------- +# watcher surfaces only event paths to R, so duplicate same-path events within +# one latency window carry no information (r-lib/watcher#55). The C API does +# not expose set_bubble_events(); as the vendored library is compiled solely +# for watcher, flip the monitor default instead. +MON_HPP="$LIBFSW_SRC/c++/monitor.hpp" +if grep -q 'bool bubble_events = true;' "$MON_HPP" 2>/dev/null; then + echo -e "${GREEN} ✓ bubble_events default already enabled${NC}" +else + sed -i.bak 's|bool bubble_events = false;|bool bubble_events = true;|' "$MON_HPP" + rm -f "$MON_HPP.bak" + if ! grep -q 'bool bubble_events = true;' "$MON_HPP"; then + echo "Error: failed to enable bubble_events in $MON_HPP" >&2 + exit 1 + fi + echo -e "${GREEN} ✓ bubble_events default enabled${NC}" +fi