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

Replaced hand-rolled with mio-based event loop. #40

Merged
merged 1 commit into from
Dec 6, 2015

Conversation

benschulz
Copy link
Contributor

This is a follow-up to #37 and hannobraun/inotify-rs#28. Currently the inotify file descriptor is closed on the thread which drops the Watcher, for which the behavior is undefined. With these changes, a message is dispatched to the event loop, causing it to shut itself down.

@@ -40,6 +40,11 @@ walker = "^1.0.0"
[target.x86_64-unknown-linux-gnu.dependencies.inotify]
version = "^0.1"

[target.x86_64-unknown-linux-gnu.dependencies.mio]
git = "https://github.com/carllerche/mio.git"
Copy link
Member

Choose a reason for hiding this comment

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

It is my understanding that libraries which depend on libraries not in Crates.io cannot be published to Crates.io. Has this changed? If not, I cannot accept this PR until it points to a Crates.io-released version of mio.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've never published a crate, but that makes a lot of sense. Do you mind if I keep this PR open in the meantime?

Copy link
Member

Choose a reason for hiding this comment

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

Sure. If there's an issue tracking this mio feature, could you link it here so we can keep track of when it makes it in a release?

Copy link
Member

Choose a reason for hiding this comment

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

The next mio milestone has a ETA for 3rd December, but I don't know whether EventedFd will be in there. I've asked on IRC for details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for looking that up/inquiring on IRC. Presumably 0.5.x will be branched off from master, so EventedFd should be included. I'll keep an eye on it either way.

Copy link
Member

Choose a reason for hiding this comment

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

IRC confirmed it :)

@benschulz
Copy link
Contributor Author

I've switched mio to version ^0.5.

The windows builds are failing ("Could not compile advapi32-sys."). Should not have anything to do with this PR.

@jmquigs
Copy link
Contributor

jmquigs commented Dec 5, 2015

I'll fix windows

@@ -43,6 +43,9 @@ version = "^0.1"
[target.x86_64-unknown-linux-musl.dependencies.inotify]
version = "^0.1"

[target.x86_64-unknown-linux-gnu.dependencies.mio]
version = "^0.5"
Copy link
Member

Choose a reason for hiding this comment

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

Also do this for target.x86_64-unknown-linux-musl

@passcod
Copy link
Member

passcod commented Dec 6, 2015

You might have to rebase this.

@benschulz
Copy link
Contributor Author

Added mio for linux-musl, squashed and rebased.

version = "^0.5"

[target.x86_64-unknown-linux-musl.dependencies.mio]
version = "^0.5"
Copy link
Member

Choose a reason for hiding this comment

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

Could you use the style as shown by other stanzas? Add mio = "^0.5" to the relevant stanzas.

@benschulz
Copy link
Contributor Author

Done.

@passcod
Copy link
Member

passcod commented Dec 6, 2015

Woohoo! Let's do this.

passcod added a commit that referenced this pull request Dec 6, 2015
Replaced hand-rolled with mio-based event loop.
@passcod passcod merged commit b0a68e2 into notify-rs:master Dec 6, 2015
@passcod
Copy link
Member

passcod commented Dec 21, 2015

Shipped in v2.5.2.

@benschulz
Copy link
Contributor Author

Cool, thanks. ;)

@benschulz benschulz deleted the inotify-race branch May 15, 2018 14:50
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.

3 participants