Add windows ReadDirectoryChanges support #39

jmquigs · 2015-11-18T18:46:59Z

Original code by @maurizi , with some fixes by me.

Fix #4

…nsmute on request pointer. - This switches the main loop to alternate between waiting for action events and blocking on SleepEx. The SleepEx block allows completion events to fire. - This also fixes a failed transmute in the completion callback. Basically instead of using the request_p casted to &mut c_void, we need to transmute it and pass that into ReadDirectoryChangesW. Otherwise, when the completion routine attempts to transmute it back, it will be crap. - Still need to fix some todos, so this is still WIP.

…dows2 Conflicts: Cargo.toml src/lib.rs

…s closed - Most of the other watchers don't panic on send failure

- Transmute converts them back into a droppable resource

- If these occur they are internal errors, so Generic seems sufficient

…efore creating files

- Otherwise files can't be renamed/deleted!

- The file or directory must exist when watch is called, since we use its type to determine how to watch it. - File watches are essentially emulated by watching the parent directory and excluding changes to anything but the target file. This allows reuse of the ReadDirectoryChangesW API.

- Windows needs a special variant of this test to deal with its limitations. - This also modifies validate_recv to return the event list, so that the calling test can check more properties

maurizi · 2015-11-18T18:53:46Z

Great to see someone finish the work I started! 👏

retep998 · 2015-11-19T02:36:42Z

src/windows.rs

@@ -0,0 +1,306 @@
+extern crate libc;


This seems entirely unused.

passcod · 2015-11-19T03:15:54Z

Cargo.toml

@@ -6,7 +6,7 @@ authors = [
  "Antti Keränen <detegr@gmail.com>",
  "Gilbert Röhrbein <gilbert@ifsr.de>",
  "Jorge Israel Peña <jorge.israel.p@gmail.com>",
-  "Michael Maurizi <mmaurizi@azavea.com>",
+  "Michael Maurizi <michael.maurizi@gmail.com>",


Add yourself in there, too :)

- Allows final read callback to fire, so that we can destroy outstanding request memory

passcod · 2015-11-19T08:59:37Z

@maurizi @retep998 waiting on your approval/testing before merge :)

jmquigs · 2015-11-19T14:18:21Z

@passcod, I'm going to investigate an alternate fix for the shutdown memory leak that @retep998 found. The sleep I have there works for the common case, but if the completion routine isn't fired before sleep exits, it still leaks memory. So if you created lots of watchers on a busy system and then shut them down, you'd probably leak some memory.

It will probably be a couple days before I can get that fix in.

- This required adding a new channel to the watcher that allows the test to observe internal changes ("Meta Events")

- Each watcher now has a a semaphore that is used to indicate when the final completion routine is called (or a read failed) - Also added some general error handling to make the code more robust in the face of deleted watch directories, etc

- Seen it fail with 5ms

jmquigs · 2015-11-19T20:52:15Z

I made some good progress on that leak here: jmquigs@02bca44. I'm using semaphores to make sure that I wait for the final completion callback on each watcher. Its not part of this PR; might be better to have a separate PR since there are so many changes here already.

maurizi · 2015-11-19T22:28:20Z

src/windows.rs

+unsafe extern "system" fn handle_event(error_code: u32, _bytes_written: u32, overlapped: LPOVERLAPPED) {
+    // TODO: Use Box::from_raw when it is no longer unstable
+    let overlapped: Box<OVERLAPPED> = mem::transmute(overlapped);
+    let request: Box<ReadDirectoryRequest> = mem::transmute(overlapped.hEvent);


I'm not sure what the minimum Rust version is for this project, but Box::from_raw has been stabilized as of 1.4

Latest stable.

I changed it to Box::from_raw

retep998 · 2015-11-20T02:11:10Z

src/windows.rs

+        // for our own purposes
+
+        let req_buf = request.buffer.as_mut_ptr() as *mut c_void;
+        let request_p: *mut c_void = mem::transmute(request);


This should be Box::into_raw

- Allows errors to be returned immediately, rather than on the event channel - Use a semaphore to wake up the server so that watch doesn't have to block for up to 500ms - Add a new metaevent to let unit test check this

jmquigs · 2015-11-21T03:14:12Z

I don't have any more pending changes for this PR. It would be nice to add a dedup to the windows watcher are some point (its kinda spammy with writes to same file, for instance), but that should wait for another PR.

passcod · 2015-11-23T10:54:05Z

@retep998 @maurizi Can either of you give the go-ahead for merging this?

jmquigs · 2015-11-25T04:21:21Z

@passcod, its fine if you aren't comfortable merging this. I'll keep the branch around in my fork anyway, since I need it for my project.

passcod · 2015-11-25T06:07:47Z

Oh, I am totally comfortable merging this, but I don't have either the expertise to assess whether it's good code nor the setup to test it (appveyor takes care of some of that, but still). I'll probably end up merging it if nobody chimes in to tell me not to in a little bit, but I still want to give time for @retep998 and/or @maurizi to give it a look over and give me a LGTM.

Add Windows support

maurizi and others added 14 commits June 13, 2015 14:39

WIP

f24f3fe

Merge branch 'master' of https://github.com/passcod/rsnotify into win…

d92ea4d

…dows2 Conflicts: Cargo.toml src/lib.rs

Windows: use full paths in reported events

1592724

Windows: use try_recv instead of select!, process all actions in loop

124652c

Windows: fix watch_dir_recommended test by not panicking if channel i…

be214c8

…s closed - Most of the other watchers don't panic on send failure

Windows: don't need to free overlapped/request in handler

1764e37

- Transmute converts them back into a droppable resource

Windows: convert watch/unwatch send errors to Error::Generic

8d2167f

- If these occur they are internal errors, so Generic seems sufficient

Windows: change test to give windows a little time to start watcher b…

12f0ae4

…efore creating files

Windows: allow write on the monitored directory

108e08c

- Otherwise files can't be renamed/deleted!

Windows: implement test for single file watcher

7e3ead6

- Windows needs a special variant of this test to deal with its limitations. - This also modifies validate_recv to return the event list, so that the calling test can check more properties

Add appveyor file

8f00aa1

appveyor: don't build the gnu targets

c536e64

retep998 reviewed Nov 19, 2015
View reviewed changes

src/windows.rs

@@ -0,0 +1,306 @@

extern crate libc;

Copy link

retep998 Nov 19, 2015

Choose a reason for hiding this comment

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

This seems entirely unused.

jmquigs added 2 commits November 18, 2015 21:50

Windows: fix some winapi imports

34b9c75

Windows: remove needless return (clippy)

672072a

passcod reviewed Nov 19, 2015
View reviewed changes

jmquigs added 2 commits November 18, 2015 22:43

Windows: wait in alertable sleep on shutdown

6c658c6

- Allows final read callback to fire, so that we can destroy outstanding request memory

Add me to contributors

3c309b7

jmquigs added 2 commits November 19, 2015 10:47

Windows: add a failing test that shows the shutdown leak

e6434a4

- This required adding a new channel to the watcher that allows the test to observe internal changes ("Meta Events")

Windows: fix shutdown leak

02bca44

- Each watcher now has a a semaphore that is used to indicate when the final completion routine is called (or a read failed) - Also added some general error handling to make the code more robust in the face of deleted watch directories, etc

Windows: increase watcher test wait time to 50ms

7dc1f55

- Seen it fail with 5ms

maurizi reviewed Nov 19, 2015
View reviewed changes

jmquigs added 2 commits November 19, 2015 19:01

Windows: eliminate warning in test

ad5e274

Windows: replace transmute with Box::from_raw

99a6fde

retep998 reviewed Nov 20, 2015
View reviewed changes

jmquigs added 2 commits November 20, 2015 22:00

Merge branch 'win-shutdown-leak' into windows2

506de31

Windows: wait for reply in watch()

e717093

- Allows errors to be returned immediately, rather than on the event channel - Use a semaphore to wake up the server so that watch doesn't have to block for up to 500ms - Add a new metaevent to let unit test check this

Windows: fix atom indent fail

50af1c6

passcod added a commit that referenced this pull request Nov 29, 2015

Merge pull request #39 from jmquigs/windows2

7e87315

Add Windows support

passcod merged commit 7e87315 into notify-rs:master Nov 29, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add windows ReadDirectoryChanges support #39

Add windows ReadDirectoryChanges support #39

jmquigs commented Nov 18, 2015

maurizi commented Nov 18, 2015

retep998 Nov 19, 2015

passcod Nov 19, 2015

passcod commented Nov 19, 2015

jmquigs commented Nov 19, 2015

jmquigs commented Nov 19, 2015

maurizi Nov 19, 2015

passcod Nov 19, 2015

jmquigs Nov 20, 2015

retep998 Nov 20, 2015

jmquigs commented Nov 21, 2015

passcod commented Nov 23, 2015

jmquigs commented Nov 25, 2015

passcod commented Nov 25, 2015

Add windows ReadDirectoryChanges support #39

Add windows ReadDirectoryChanges support #39

Conversation

jmquigs commented Nov 18, 2015

maurizi commented Nov 18, 2015

retep998 Nov 19, 2015

Choose a reason for hiding this comment

passcod Nov 19, 2015

Choose a reason for hiding this comment

passcod commented Nov 19, 2015

jmquigs commented Nov 19, 2015

jmquigs commented Nov 19, 2015

maurizi Nov 19, 2015

Choose a reason for hiding this comment

passcod Nov 19, 2015

Choose a reason for hiding this comment

jmquigs Nov 20, 2015

Choose a reason for hiding this comment

retep998 Nov 20, 2015

Choose a reason for hiding this comment

jmquigs commented Nov 21, 2015

passcod commented Nov 23, 2015

jmquigs commented Nov 25, 2015

passcod commented Nov 25, 2015