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

Add debouncer #286

Merged
merged 8 commits into from
Aug 6, 2022
Merged

Add debouncer #286

merged 8 commits into from
Aug 6, 2022

Conversation

0xpr03
Copy link
Member

@0xpr03 0xpr03 commented Mar 14, 2021

Add debouncer back to v5.

Related #249

@0xpr03 0xpr03 changed the title Add debouncer mockup Add debouncer Mar 14, 2021
@0xpr03 0xpr03 force-pushed the debouncer branch 2 times, most recently from 9746530 to b64cdf1 Compare March 14, 2021 02:14
@0xpr03
Copy link
Member Author

0xpr03 commented Mar 14, 2021

Open questions:

  • Do we want to add multiple-receiver functionality?
  • What is the priority for any/other events ?
  • Do we allow much configuration in light of "Any" on invalid configurations overriding anything?
  • Do we unify the debouncer and configuration into one thing ? Do we want to split it up into separate APIs?
  • Do we want to spawn another thread for timing? Otherwise we'll need more redesign.
  • What exactly would be the best debounced event kind? I currently leave out AnyMap and simply use the first t of the EventKind(t) provided, ignoring in-depth details such as the exact ModifyKind, rendering the actual modify events ModifyKind useless.
  • The original API accepted a simple Receiver, is there any use case for accepting a sender instead of spawning the watcher internally and only returning a receiver?

@0xpr03
Copy link
Member Author

0xpr03 commented Apr 16, 2021

Another I just realized: We need an idea how we handle configuration changes for debouncer and immediate watchers. Currently we don't care at all. Historically every debouncer init was implemented at the specific notify modules (inotify, fsevent..).

What I realized: we don't actually implement any of the current config options. Or am I wrong ? Searching for PreciseEvents doesn't emit one hit across any of the specific implementations. Also from all the config options exactly one is relevant to the specific implementations of the immediate watchers: PreciseEvents, everything else is related to the debouncer. Thus the whole ".config()" operation is nearly irrelevant for immediate watchers.

cc @JohnTitor

@passcod
Copy link
Member

passcod commented Oct 28, 2021

Notes from implementing the watchexec 2.0 debouncer:

  • An important consideration in watchexec was that the filtering would apply before/during debouncing. So events are discarded if filtered out, and end up not affecting the debouncing. To me that's the biggest hurdle this module would have: if end user wants to filter events at all it's more correct to do it during debounce, but providing an interface for that within the module is... too big a scope I think

  • Looking through notify while I implemented-- I don't think config is really used at all, and I'm dubious about notice and precise events now. I was trying to scope notify a lot larger, which I think made the design suffer. But I could be misremembering / not recalling concerns, idk.

  • The main advantages of a notify built-in debouncer imo are:

    • to provide a useful interface for the common/naive case
    • to resolve renames

    That second one is useful all the time, the first less so. Unsure if they could be decoupled in some way.

  • Initially I recorded times/instants for events received/emitted, but I discarded that pretty fast. However, it's probably that I don't see the need in watchexec rather than it not being useful generally.

  • Notify's debouncer historically groups by major event kind and aggregates into a synthetic type; watchexec's doesn't. There was some discussion on here about the common usecase being "did something change" rather than "did something get added/removed/modified"; that's what I embraced. Watchexec's debouncer output is a Vec<Event>, basically.

For reference, algorithm I implemented (simplified):

let throttle: Duration = unimplemented!("input: debouncer interval");
let mut last = Instant::now();
let mut set = Vec::new();

loop {
    let maxtime = if set.is_empty() {
        Duration::from_secs(u64::MAX)
    } else {
        throttle.saturating_sub(last.elapsed())
    };

    if maxtime.is_zero() {
        if set.is_empty() {
            last = Instant::now();
            continue;
        } else {
            // we're out of the debouncing
        }
    } else {
        match timeout(maxtime, events.recv()).await {
            Err(_timeout) => continue,
            Ok(None) => break, // exit
            Ok(Some(event)) => {
                // filtering applied here, if fail reset loop

                if set.is_empty() {
                    last = Instant::now();
                }

                set.push(event);

                if last.elapsed() < throttle {
                    continue;
                }
            }
        }
    }

    // we're out of the debouncing, pass event set to consumer
    last = Instant::now();
    let pass_this = set.drain(..).collect();
}

@IWANABETHATGUY
Copy link

IWANABETHATGUY commented Dec 2, 2021

I am curious when will this be merged ?

@0xpr03
Copy link
Member Author

0xpr03 commented Dec 2, 2021

Once I've got time to fully undertake this as there is a lot I want to get right or at least decide actively to not include it, as you can read above. Which due to private things going on could take until march next year. I consider v4 to be pretty stable, so feel free to use that one.

@0xpr03 0xpr03 force-pushed the debouncer branch 3 times, most recently from e515d57 to 8f8280c Compare June 16, 2022 22:32
@0xpr03
Copy link
Member Author

0xpr03 commented Jun 17, 2022

This is the tiniest debouncer I could imagine being useful. Only Any events are delivered and AnyContinuous for ongoing events past the timeout.

Things I don't like:

  • you can't change the tick rate
  • you can't change the timeout
  • you can't provide the backend

@0xpr03
Copy link
Member Author

0xpr03 commented Jun 17, 2022

The real problem with all of these three is how we pass the configuration stuff back and forth. User -> backend (config), User -> debouncer (config), backend -> debouncer (events, errors), debouncer -> User (events, errors)

Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too familiar with it but the implementation looks great to me with a nit!

src/debouncer.rs Outdated
) -> Result<(Receiver<DebounceChannelType>, RecommendedWatcher), Error> {
let data = DebounceData::default();

let tick_div = 4;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'd like to manage this value as const and declare it on the module scope.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

understandable, had a moment of wrangling with myself whether I'd want that as top module declaration

@IWANABETHATGUY

This comment was marked as spam.

@CrendKing

This comment was marked as abuse.

@0xpr03 0xpr03 added this to the 5.0.0 milestone Aug 2, 2022
@0xpr03 0xpr03 marked this pull request as draft August 2, 2022 12:13
@0xpr03 0xpr03 force-pushed the debouncer branch 7 times, most recently from 8a1d6e1 to 27634a4 Compare August 2, 2022 13:58
@0xpr03
Copy link
Member Author

0xpr03 commented Aug 2, 2022

This got a little bit bigger now. Fixing the audit issues and decoupling the debouncer from notify core.

@0xpr03 0xpr03 force-pushed the debouncer branch 2 times, most recently from 6736894 to 3f23588 Compare August 2, 2022 20:00
@0xpr03
Copy link
Member Author

0xpr03 commented Aug 2, 2022

As good as finished.
One open point is how I could enable custom watcher options (see pollwatcher) with this system ? Currently I can't think of a good combination. The optional crossbeam feature should be enhanced when we land an additional feature in notify itself to allow disabling crossbeam entirely for tokio compatibility.

@0xpr03 0xpr03 marked this pull request as ready for review August 2, 2022 21:55
@0xpr03 0xpr03 force-pushed the debouncer branch 3 times, most recently from 4d7023b to 00c7cba Compare August 4, 2022 22:46
@0xpr03 0xpr03 merged commit cbcc23e into main Aug 6, 2022
@0xpr03 0xpr03 deleted the debouncer branch August 9, 2022 16:38
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

Successfully merging this pull request may close these issues.

6 participants