Name all threads #383
Name all threads #383
Conversation
|
Audit fails - but seems unrelated to my changes. |
|
The idea itself makes sense to me! I'd like to hear @0xpr03's thoughts about event name bikeshedding. |
|
Feel free to suggest different names, and I'll update it. I figured having a common prefix for them would be nice, so it's easy to see they're from this crate. |
|
Seems they're out until Feb. 6th, wait for a while :) |
There was a problem hiding this comment.
Hey thanks for the PR.
I thought a little bit about the naming and I think adding something like "event loop" would be good, if we want to drop a hint for people debugging / inspecting the threads.
I marked two things because there is a lot of indentation noise I'd like to avoid if possible. If that's from rustfmt I guess we'll have to live with it.
P.S. The audit fails are just outdated dependencies in the tide-web example, nothing we can currently do about that..
| let kind = EventKind::Modify(meta); | ||
| let ev = Event::new(kind).add_path(watch.clone()); | ||
| event_handler(Ok(ev)); | ||
| for ( |
There was a problem hiding this comment.
is all this indentation noise from rustfmt ?
There was a problem hiding this comment.
Yeah - it just jumps in a bit because the scope block indents one further.
| { | ||
| let path = entry.path(); | ||
| } else { | ||
| let depth = |
| }) | ||
| .unwrap(); | ||
| let _ = thread::Builder::new() | ||
| .name("notify-rs test-race-with-remove-dir".to_string()) |
There was a problem hiding this comment.
I'm not sure we want thread names in tests, as it'll just add more code noise, but I guess its fine
There was a problem hiding this comment.
I had the same doubts - but I figured why not.
src/fsevent.rs
Outdated
| } | ||
| }); | ||
| let thread_handle = thread::Builder::new() | ||
| .name("notify-rs fsevents".to_string()) |
It's a small change to give a name to all threads notify creates, this way it's easy to pinpoint where they come from in a debugger and profiler.
Edit: cargo fmt is a separate commit so it's a bit easier to see the changes.
Edit 2:
cargo build --workspace --all-targetsdoesn't seem to build cleanly, andcargo clippy --workspace --all-targetsalso doesn't seem to be clean on main?Edit 3: in our own codebase we've added this to clippy.toml which could be interesting to add here as well.