improve documentation on std::thread::sleep #54646

vn971 · 2018-09-28T16:39:06Z

No description provided.

rust-highfive · 2018-09-28T16:39:09Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aidanhs (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

nagisa · 2018-09-28T16:51:55Z

This is wrong. The function will not return early and the implementation reflects it. It may result in the backing call being called multiple times, though.

vn971 · 2018-09-28T17:07:19Z

@nagisa oh, so it's instead a guarantee that Rust provides.
I wonder why it's listed under "Platform-specific behavior" then though. Does it feel like this section needs some re-wording? https://doc.rust-lang.org/std/thread/fn.sleep.html#platform-specific-behavior

sfackler · 2018-09-28T17:14:38Z

It seems like it would be less confusing for it to just say that it guarantees that the function will not return for at least the specified duration.

nagisa · 2018-09-28T17:22:30Z

It is under the platform-specific behaviour for unix because the function does not behave the same way as the underlying syscall would. On Windows, for example, the underlying syscall does not sporadically wake.

I agree that it would make sense to move the guarantee of lack of spurious wakeups to the regular documentation.

vn971 · 2018-09-28T19:42:46Z

@nagisa @sfackler updated. Does that read better now?

sfackler · 2018-09-28T20:39:11Z

lgtm, but cc @rust-lang/docs for their sign-off.

frewsxcv · 2018-09-29T01:49:12Z

src/libstd/thread/mod.rs

-/// On Unix platforms this function will not return early due to a
-/// signal being received or a spurious wakeup.
+/// On Unix platforms this function may invoke multiple syscalls
+/// in case of a signal being received or a spurious wakeup.


On Unix platforms this function may invoke multiple syscalls in case of a signal being received or a spurious wakeup.

I don't quite understand why we're talking about system calls here. Can someone explain how it's relevant?

@frewsxcv The way how "sleep" is usually implemented is telling the OS to put a thread to sleep. This is done via a "syscall". "sleep" is so often implemented via syscall that some people may even assume it's the same. And in many languages this is basically the same.

Also, making more than one syscall could probably have some performance impact in very specific cases -- though I'm not entirely sure about that.

Thanks for the explanation! Why should the reader care how many system calls are invoked per platform? Just trying to figure out what the value-add is for this section.

@frewsxcv I guess the value-add is clarity.

Some people are just too used to the "old"/"usual" behavior. Even JVM, which poses itself as cross-platform, still makes only one underlying sleeping syscall, which results in the specific behavior.

How do you feel about this text?

> On Unix platforms, the [underlying `sleep` system call][syscall] can sporadically > wake early. To ensure we sleep for at least the specified duration, this function > may invoke that system call multiple times. > > [syscall]: http://man7.org/linux/man-pages/man3/sleep.3.html

I think this provides some more context to the reader why we're talking about system calls and why this is platform-specific. Thoughts?

Sounds interesting. Some remarks though:

the underlying syscall is nanosleep: http://man7.org/linux/man-pages/man2/nanosleep.2.html (note the number -- it's "2" for systemcalls. Number "3" is for "library calls" -- basically the C interface).

the thread can as well wake up due to a signal received (as documented in the manual). That needs to be mentioned as well.

I'll return to this a bit later.

Another attempt:

> On Unix platforms, the [underlying `nanosleep` system call][syscall] can > spuriously wake early or wake early from a signal interruption. To > ensure the sleep occurs for at least the specified duration, this function > may invoke that system call multiple times. > > [syscall]: http://man7.org/linux/man-pages/man2/nanosleep.2.html

@frewsxcv but then again if I think more about the problem, it seems that the exact system call might be Linux-specific, whereas the start of paragraph says "Unix".
I've pushed an update taking the idea and wording of your proposal, please take a look.

Havvy · 2018-09-29T05:09:16Z

The documentation should match what it says in the sleep function right after it. Both of them should also define what "putting a thread to sleep" actually means, but that's doable in a separate PR.

vn971 · 2018-09-30T19:34:07Z

@Havvy wait, was that a review request? Can you please point out where exactly would you like a fix to be made? // I'm new to Rust-s documentation rules.

Havvy · 2018-10-01T00:03:40Z

The sleep_ms fn is located directly after sleep in the code. As they're similar fns, they should have similar documentation.

vn971 · 2018-10-01T08:02:20Z

@Havvy thanks, I see. Updated.

vn971 · 2018-10-06T09:02:40Z

Any updates? All review comments are addressed I think.

frewsxcv · 2018-10-06T11:49:00Z

Any updates? All review comments are addressed I think.

Thanks for being patient with this 🙏 Once we finish up that comment thread I'll approve this ✅

frewsxcv · 2018-10-16T12:22:09Z

looks great, thanks for being patient!

@bors r+ rollup

bors · 2018-10-16T12:22:10Z

📌 Commit 7a0fa95 has been approved by frewsxcv

vn971 · 2018-10-16T12:26:16Z

Thanks for all the feedback as well -- it was very helpful.

improve documentation on std::thread::sleep

Rollup of 18 pull requests Successful merges: - #54646 (improve documentation on std::thread::sleep) - #54933 (Cleanup the rest of codegen_llvm) - #54964 (Run both lldb and gdb tests) - #55016 (Deduplicate some code and compile-time values around vtables) - #55031 (Improve verify_llvm_ir config option) - #55050 (doc std::fmt: the Python inspiration is already mentioned in precedin…) - #55077 (rustdoc: Use dyn keyword when rendering dynamic traits) - #55080 (Detect if access to localStorage is forbidden by the user's browser) - #55090 (regression test for move out of borrow via pattern) - #55102 (resolve: Do not skip extern prelude during speculative resolution) - #55104 (Add test for #34229) - #55111 ([Rustc Book] Explain --cfg's arguments) - #55122 (Cleanup mir/borrowck) - #55127 (Remove HybridBitSet::dummy) - #55128 (Fix LLVMRustInlineAsmVerify return type mismatch) - #55142 (miri: layout should not affect CTFE checks (outside of validation)) - #55151 (Cleanup nll) - #55161 ([librustdoc] Disable spellcheck for search field)

fix std::thread::sleep typo

1a605a6

rust-highfive assigned aidanhs Sep 28, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 28, 2018

update wording for std::thread::sleep

049ccbb

frewsxcv reviewed Sep 29, 2018

View reviewed changes

vn971 changed the title ~~fix std::thread::sleep typo~~ improve documentation on std::thread::sleep Sep 29, 2018

dtolnay mentioned this pull request Oct 1, 2018

typo in std::thread::sleep #54644

Closed

update wording for thread::sleep

b63517a

improve docs on thread::sleep

7a0fa95

frewsxcv approved these changes Oct 16, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 16, 2018

kennytm mentioned this pull request Oct 18, 2018

Rollup of 18 pull requests #55171

Merged

kennytm added a commit to kennytm/rust that referenced this pull request Oct 18, 2018

Rollup merge of rust-lang#54646 - vn971:fix_std_thread_sleep, r=frewsxcv

e4ac447

improve documentation on std::thread::sleep

bors merged commit 7a0fa95 into rust-lang:master Oct 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve documentation on std::thread::sleep #54646

improve documentation on std::thread::sleep #54646

vn971 commented Sep 28, 2018 •

edited

Loading

rust-highfive commented Sep 28, 2018

nagisa commented Sep 28, 2018

vn971 commented Sep 28, 2018 •

edited

Loading

sfackler commented Sep 28, 2018

nagisa commented Sep 28, 2018

vn971 commented Sep 28, 2018

sfackler commented Sep 28, 2018

frewsxcv Sep 29, 2018

vn971 Sep 29, 2018

frewsxcv Sep 30, 2018

vn971 Sep 30, 2018

frewsxcv Oct 6, 2018

vn971 Oct 6, 2018 •

edited

Loading

frewsxcv Oct 6, 2018

vn971 Oct 11, 2018

Havvy commented Sep 29, 2018

vn971 commented Sep 30, 2018

Havvy commented Oct 1, 2018

vn971 commented Oct 1, 2018

vn971 commented Oct 6, 2018

frewsxcv commented Oct 6, 2018

frewsxcv commented Oct 16, 2018

bors commented Oct 16, 2018

vn971 commented Oct 16, 2018

improve documentation on std::thread::sleep #54646

improve documentation on std::thread::sleep #54646

Conversation

vn971 commented Sep 28, 2018 • edited Loading

rust-highfive commented Sep 28, 2018

nagisa commented Sep 28, 2018

vn971 commented Sep 28, 2018 • edited Loading

sfackler commented Sep 28, 2018

nagisa commented Sep 28, 2018

vn971 commented Sep 28, 2018

sfackler commented Sep 28, 2018

frewsxcv Sep 29, 2018

Choose a reason for hiding this comment

vn971 Sep 29, 2018

Choose a reason for hiding this comment

frewsxcv Sep 30, 2018

Choose a reason for hiding this comment

vn971 Sep 30, 2018

Choose a reason for hiding this comment

frewsxcv Oct 6, 2018

Choose a reason for hiding this comment

vn971 Oct 6, 2018 • edited Loading

Choose a reason for hiding this comment

frewsxcv Oct 6, 2018

Choose a reason for hiding this comment

vn971 Oct 11, 2018

Choose a reason for hiding this comment

Havvy commented Sep 29, 2018

vn971 commented Sep 30, 2018

Havvy commented Oct 1, 2018

vn971 commented Oct 1, 2018

vn971 commented Oct 6, 2018

frewsxcv commented Oct 6, 2018

frewsxcv commented Oct 16, 2018

bors commented Oct 16, 2018

vn971 commented Oct 16, 2018

vn971 commented Sep 28, 2018 •

edited

Loading

vn971 commented Sep 28, 2018 •

edited

Loading

vn971 Oct 6, 2018 •

edited

Loading