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

Name all threads #383

Merged
merged 4 commits into from
Feb 14, 2022
Merged

Name all threads #383

merged 4 commits into from
Feb 14, 2022

Conversation

Jasper-Bekkers
Copy link
Contributor

@Jasper-Bekkers Jasper-Bekkers commented Feb 1, 2022

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-targets doesn't seem to build cleanly, and cargo clippy --workspace --all-targets also 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.

disallowed-methods = [
    # We require all of our threads to be named, use std::thread::Builder instead
    # to provide a name for the thread.
    "std::thread::spawn",
]

@Jasper-Bekkers
Copy link
Contributor Author

Audit fails - but seems unrelated to my changes.

@JohnTitor
Copy link
Member

The idea itself makes sense to me! I'd like to hear @0xpr03's thoughts about event name bikeshedding.

@Jasper-Bekkers
Copy link
Contributor Author

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.

@JohnTitor
Copy link
Member

Seems they're out until Feb. 6th, wait for a while :)

Copy link
Member

@0xpr03 0xpr03 left a comment

Choose a reason for hiding this comment

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

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 (
Copy link
Member

Choose a reason for hiding this comment

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

is all this indentation noise from rustfmt ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - it just jumps in a bit because the scope block indents one further.

{
let path = entry.path();
} else {
let depth =
Copy link
Member

Choose a reason for hiding this comment

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

same here for the newline

})
.unwrap();
let _ = thread::Builder::new()
.name("notify-rs test-race-with-remove-dir".to_string())
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we want thread names in tests, as it'll just add more code noise, but I guess its fine

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 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())
Copy link
Member

Choose a reason for hiding this comment

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

maybe "notify-rs fsevent loop" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@0xpr03 0xpr03 left a comment

Choose a reason for hiding this comment

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

thanks

@0xpr03 0xpr03 merged commit 59efaa2 into notify-rs:main Feb 14, 2022
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