Frequently written files may get debounced indefinitely #183

sjoshid · 2019-03-05T00:17:54Z

This PR is for issue 146. It contains:

A new debounced event called OnGoingWrite which is optional by default to keep existing clients unchanged.
This event is fired when a file is bombarded with WRITE events.
Once the actual WRITE event is fired, this event is unscheduled.

I have made changes for linux and macos platforms. I haven't made for Windows because I don't have access to one. But I'll find a way to get it.

2) When Write is emitted, we disable onGoingWrite.

…ventLoop's channel 2) This will then set on_going_write's delay in WatchTimer.

2) Refactoring.

sjoshid · 2019-03-06T15:01:28Z

@passcod
I was able to implement the Windows version of this. Let me know what you think of this PR or want me to make more changes.
Im looking into the failing test case for Rust 1.26.1 for Mac.

passcod · 2019-03-07T20:23:50Z

What's with all the commented out code?

sjoshid · 2019-03-08T01:44:14Z

Remnants of my previous attempts to fix this. Cleaned them up.

sjoshid · 2019-03-11T02:09:38Z

@passcod
I got rust nightly on macos but the Travis's failing test case poll_watch_file doesn't fail in my local. I also saw that there is another PR with a similar failing test case. Is there anything I need to do fix the failing test case or this is a known problem that can be ignored?

passcod · 2019-03-11T02:22:14Z

Yeah, tests are flaky, especially on Travis (likely to do with the virtualisation or something... testing three times per platform tends to help eliminate things out). I usually get them to retry, but I missed that one as I haven't had the time to review this PR in depth last week.

sjoshid · 2019-03-11T13:29:51Z

Looks like tests passed. Yaay!! I’m also going to add few more tests cases for the new event depending on your your feedback.

_{Sent with GitHawk}

sjoshid · 2019-03-14T20:44:46Z

@passcod Did you get a change to look at this PR? I have another PR that is waiting for this. There is no rust at all. :)

passcod · 2019-03-14T20:49:02Z

sorry, no. I'm focused on an upcoming release at work, will have time next week

…

On Fri, 15 Mar 2019 at 09:44, Sujit Joshi ***@***.***> wrote: @passcod <https://github.com/passcod> Did you get a change to look at this PR? I have another PR <xi-editor/xi-editor#1011> that is waiting for this. There is no rust at all. :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#183 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJgiwb796xfjRVC71mpQPybPwDa9jqpks5vWrS_gaJpZM4bde5L> .

passcod

General:

needs the documentation

src/lib.rs

src/debounce/mod.rs

passcod

Looks good! Some more general things:

The correct spelling of the word is "ongoing", no spaces and no hyphens, so all the methods should be set_ongoing_... and the types should be OngoingWrite, ONGOING_WRITE, etc.
This falls under "documentation", but as a note, we should provide some guidance as to what the duration should be, especially in relation to the debounce duration.
The method set_ongoing_write_duration is a bit awkward. What that method does is both "switch on the OngoingWrite event, and configure the interval duration" and "re-configure the interval duration" depending on context. The current name implies only the configuring part and it's not obvious that it may be turning on the feature.
We provide a way to turn ongoing write events on, but no way to turn them off. That should probably be allowed.

There's several ways to do this, but I think what would be most forward-looking here is adding a new configure(&mut self, option: Config) -> Result<()>; method and making a Config enum that would initially look like:

enum Config {
	OngoingWrites(Option<Duration>),
}

So usage would be like:

watcher.configure(Config::OngoingWrites(Some(Duration::from_ms(50))))?;
watcher.configure(Config::OngoingWrites(None))?;

That takes care of the method name and brings it in line with the other ones (single verbs), and also means that any further runtime options can be added there without more bikeshedding.

Functionally, the trait would contain a default implementation that just returns Ok(()) for everything, and most everything else would keep working the same.

sjoshid · 2019-03-23T23:04:46Z

All suggestions are valid and are included. I liked the idea of making is configurable. Made changes accordingly.

passcod

Alright! That's looking good. I'll merge later today (or tomorrow depending on how life goes).

sjoshid · 2019-03-25T13:57:38Z

src/lib.rs

@@ -611,6 +615,25 @@ pub trait Watcher: Sized {
    /// Returns an error in the case that `path` has not been watched or if removing the watch
    /// fails.
    fn unwatch<P: AsRef<Path>>(&mut self, path: P) -> Result<()>;
+
+    /// Configure notify with Configs.
+    fn configure(&self, option: Config) -> Result<()> {


Thinking more about this...should I make this a collection of Config rather than just a Config?

passcod · 2019-03-25T19:25:28Z

That would probably complicate things. Calling configure multiple times is fine, but accepting a collection would mean doing deduplication or conflict detection or have odd behaviour (what should passing Config::OngoingWrite(Some(...)) and Config::OngoingWrite(None) do?) That can be added/changed later as needed.

…

On Tue, 26 Mar 2019, 02:57 Sujit Joshi, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/lib.rs <#183 (comment)>: > @@ -611,6 +615,25 @@ pub trait Watcher: Sized { /// Returns an error in the case that `path` has not been watched or if removing the watch /// fails. fn unwatch<P: AsRef<Path>>(&mut self, path: P) -> Result<()>; + + /// Configure notify with Configs. + fn configure(&self, option: Config) -> Result<()> { Thinking more about this...should I make this a collection of Config rather than just a Config? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#183 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJgi8CTXwj3Eelw-I9oEiI44IXrbNerks5vaNXTgaJpZM4bde5L> .

sjoshid · 2019-03-25T23:05:56Z

Fair enough. I’ll keep this the way it is. On Mon, Mar 25, 2019 at 3:25 PM Félix Saparelli <[email protected]> wrote:

That would probably complicate things. Calling configure multiple times is fine, but accepting a collection would mean doing deduplication or conflict detection or have odd behaviour (what should passing Config::OngoingWrite(Some(...)) and Config::OngoingWrite(None) do?) That can be added/changed later as needed. On Tue, 26 Mar 2019, 02:57 Sujit Joshi, ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In src/lib.rs > <#183 (comment)>: > > > @@ -611,6 +615,25 @@ pub trait Watcher: Sized { > /// Returns an error in the case that `path` has not been watched or if removing the watch > /// fails. > fn unwatch<P: AsRef<Path>>(&mut self, path: P) -> Result<()>; > + > + /// Configure notify with Configs. > + fn configure(&self, option: Config) -> Result<()> { > > Thinking more about this...should I make this a collection of Config > rather than just a Config? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#183 (review) >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/AAJgi8CTXwj3Eelw-I9oEiI44IXrbNerks5vaNXTgaJpZM4bde5L > > . > — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#183 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGcDclRXcMWvnCKZVa3_nOKDoWnhO_Tcks5vaSKpgaJpZM4bde5L> .

-- Thank you. - Sujit Joshi.

sjoshid · 2019-03-28T10:20:07Z

Thanks for merging. 👍

sjoshid added 10 commits February 24, 2019 12:28

WPI: First attempt at on_going_write

c3bd08a

WPI: Able to emit on_going_write. Needs lot of refactoring.

3646241

WPI: Able to emit on_going_write. Needs lot of refactoring.

3c35d62

1) Set onGoingWrite event only when Write is delayed.

8b6037a

2) When Write is emitted, we disable onGoingWrite.

Scheduling on_going_write in WatchTimer

339b0fc

1) Created new EventLoopMsg, OnGoingWrite which will be shoved down E…

c5d86c5

…ventLoop's channel 2) This will then set on_going_write's delay in WatchTimer.

Firing on_going_write from Debounce itself.

7052a85

1) Adding macos changes.

4e6beb3

2) Refactoring.

1) Refactoring and docs.

f659050

1) Removing println.

c531f72

sjoshid changed the title ~~PR for https://github.com/passcod/notify/issues/146~~ Frequently written files may get debounced indefinitely Mar 5, 2019

- Implementing on_going_write event in Windows.

d3e2ad1

Removing comments.

ccd1a15

passcod reviewed Mar 22, 2019

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

passcod reviewed Mar 22, 2019

View reviewed changes

src/debounce/mod.rs Outdated Show resolved Hide resolved

sjoshid added 2 commits March 22, 2019 17:30

Changes as per code review.

a2ebcfd

Refactoring as per code review.

a6f342d

passcod requested changes Mar 22, 2019

View reviewed changes

sjoshid added 2 commits March 23, 2019 14:07

Renaming "on_going" to "ongoing"

4a69c4b

Documentation and some more refactoring.

8c482a0

passcod approved these changes Mar 23, 2019

View reviewed changes

sjoshid commented Mar 25, 2019

View reviewed changes

passcod merged commit 0a740eb into notify-rs:main Mar 28, 2019

passcod added a commit that referenced this pull request Mar 28, 2019

[docs] Write changelog for #180, #183, #184

8ba7653

sjoshid mentioned this pull request Mar 28, 2019

tail toggle? xi-editor/xi-editor#1011

Closed

passcod added this to the 5.0.0 milestone Mar 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Frequently written files may get debounced indefinitely #183

Frequently written files may get debounced indefinitely #183

sjoshid commented Mar 5, 2019 •

edited

Loading

sjoshid commented Mar 6, 2019

passcod commented Mar 7, 2019

sjoshid commented Mar 8, 2019

sjoshid commented Mar 11, 2019

passcod commented Mar 11, 2019

sjoshid commented Mar 11, 2019

sjoshid commented Mar 14, 2019

passcod commented Mar 14, 2019 via email

passcod left a comment •

edited

Loading

passcod left a comment •

edited

Loading

sjoshid commented Mar 23, 2019 •

edited

Loading

passcod left a comment

sjoshid Mar 25, 2019

passcod commented Mar 25, 2019 via email

sjoshid commented Mar 25, 2019 via email

sjoshid commented Mar 28, 2019

Frequently written files may get debounced indefinitely #183

Frequently written files may get debounced indefinitely #183

Conversation

sjoshid commented Mar 5, 2019 • edited Loading

sjoshid commented Mar 6, 2019

passcod commented Mar 7, 2019

sjoshid commented Mar 8, 2019

sjoshid commented Mar 11, 2019

passcod commented Mar 11, 2019

sjoshid commented Mar 11, 2019

sjoshid commented Mar 14, 2019

passcod commented Mar 14, 2019 via email

passcod left a comment • edited Loading

Choose a reason for hiding this comment

passcod left a comment • edited Loading

Choose a reason for hiding this comment

sjoshid commented Mar 23, 2019 • edited Loading

passcod left a comment

Choose a reason for hiding this comment

sjoshid Mar 25, 2019

Choose a reason for hiding this comment

passcod commented Mar 25, 2019 via email

sjoshid commented Mar 25, 2019 via email

sjoshid commented Mar 28, 2019

sjoshid commented Mar 5, 2019 •

edited

Loading

passcod left a comment •

edited

Loading

passcod left a comment •

edited

Loading

sjoshid commented Mar 23, 2019 •

edited

Loading