Segfault when watcher is moved. #33
Comments
|
Here's how I'm using rsnotify: fn watch(file: &str) {
use notify::{RecommendedWatcher, Error, Watcher};
use std::sync::mpsc::channel;
use std::thread;
// Create a channel to receive the events.
let (tx, rx) = channel();
let mut watcher = notify::new(tx).unwrap();
watcher.watch(file).unwrap();
thread::spawn(move || {
match rx.recv() {
_ => println!("changes!")
}
});
}
fn main() {
watch("./test.ares");
} |
passcod
added
A-bug
Z-needs implementation
|
Pinging @octplane on this. |
|
Can't reproduce using this very code: extern crate notify;
fn watch(file: &str) {
use notify::Watcher;
use std::sync::mpsc::channel;
use std::thread;
// Create a channel to receive the events.
let (tx, rx) = channel();
let mut watcher = notify::new(tx).unwrap();
watcher.watch(file).unwrap();
let _ = thread::spawn(move || {
loop {
match rx.recv() {
_ => println!("changes!")
}
}
}).join();
}
fn main() {
watch("./test.ares");
}I'm using |
|
@octplane Ah, sorry about that, it looks like the issue was based around how threading and dropping the watcher interact. Here's my repro using extern crate notify;
fn watch(file: &str) -> notify::FsEventWatcher {
use notify::Watcher;
use std::sync::mpsc::channel;
use std::thread;
// Create a channel to receive the events.
let (tx, rx) = channel();
let mut watcher = notify::new(tx).unwrap();
watcher.watch(file).unwrap();
// Don't join this thread
thread::spawn(move || {
loop {
match rx.recv() {
_ => println!("changes!")
}
}
});
watcher
}
fn main() {
// Don't let the watcher drop
let _watcher = watch("./test.ares");
// in my real application, this sleep-loop
// is actually a webserver that is running
loop {
::std::thread::sleep_ms(20);
}
}LLDB stacktrace |
|
Looks like the rsnotify wrapping around FSEvent has a bug, cf rust-lang/rust#29201 (comment) @passcod , I'm not much familiar with the overall architecture of this piece of code. Are you? This is likely to change some of the rsnotify internals a bit.. |
TyOverby
changed the title
|
I've updated the title. |
|
For ref: https://github.com/passcod/rsnotify/blob/master/src/fsevent.rs#L132-L164 I didn't write this code, actually, and I'm not quite sure what it does, or why such a large unsafe block is needed. If we remove the What kind of internal reshuffle are you envisioning? |
|
:( it was writed by me. @passcod |
FsEventWatch.context:: Option<StreamContextInfo> is used for passing channels to watch thread. When FsEventWatch moves, its address changes, so c callback will fail. Fix: add Box<> wrapper to StreamContextInfo
|
My bad. PRed, waiting for review. Diagnose:
Here, the |
|
@passcod The large unsafe block does following:
The unsafe should be splited to two(outside thread and inside thread). Maybe the info structs should be wraped into |
No worries! All of us overlook things, it is no big deal. Bugs happen. Thank you for the swift response and contribution of a fix :) |
|
Yeah, no worries @andelf , that you found a way to fix the issue is very cool! |
|
OK! :) |
|
No longer segfaults in my application! Thanks! |
|
Apparently still occurs, see watchexec/cargo-watch#22. |
passcod
removed
the
Z-needs implementation
|
I can reproduce the issue with cargo-watch @ master. However, the original test-case do not fail, so I think we will have to take |
|
Ok, it's possible to reproduce the crash by issuing 2 watchs command. |
|
I have updated my test case. It exhibits something going wrong immediately. Please see https://github.com/octplane/rsnotify-33 Clearly, the channel is shut down way to early. Note that if you move the main loop{} in watch() it starts behaving correctly. |
|
I set a breakpoint on function rust-panic in LLDB and triggered a file change: |
Is enough to crash the watcher at first change, too. I propose we move to a new bug. |
|
Also, my first investigation shows that the One way to try and fix is to ensure that |
Fix race condition Closes #33 (again). Fixes watchexec/cargo-watch#22.
I have a program written using rsnotify 2.3.3 that segfaults when I
toucha file that I'm watching.Here's the backtrace from lldb
The text was updated successfully, but these errors were encountered: