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

Clarify docs regarding sleep of zero duration #79849

Merged
merged 2 commits into from
Feb 10, 2021

Conversation

Digital-Chaos
Copy link
Contributor

Clarify that the behaviour of sleep() when given a duration of zero is actually platform specific.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 9, 2020
@Digital-Chaos Digital-Chaos changed the title Update mod.rs Clarify docs regarding sleep of zero duration Dec 9, 2020
@jyn514 jyn514 added A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 9, 2020
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 

@jyn514
Copy link
Member

jyn514 commented Dec 23, 2020

 tidy error: /checkout/library/std/src/thread/mod.rs:778: trailing whitespace
tidy error: /checkout/library/std/src/thread/mod.rs:779: trailing whitespace
tidy error: /checkout/library/std/src/thread/mod.rs:780: trailing whitespace

@jyn514
Copy link
Member

jyn514 commented Dec 23, 2020

@Digital-Chaos I'm not the right reviewer, I'm not on the libs team.

@m-ou-se
Copy link
Member

m-ou-se commented Jan 9, 2021

We've discussed this PR in the library team meeting a few days ago, and felt like an overview of what happens on the different platforms just like for SystemTime::now would be the right thing to do here. This wouldn't be a hard promise, but simply documentation about the current implementation. (Possibly with a note saying that if you rely on the behaviour of sleep(0) for a specific platform, you should probably call the platform-specific function directly.)

@m-ou-se m-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed I-nominated S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2021
@Digital-Chaos
Copy link
Contributor Author

Thanks for the feedback! As we are going for the extra docs route, will the change in his PR suffice or do we need more detail?

@m-ou-se
Copy link
Member

m-ou-se commented Jan 13, 2021

Ah sorry; to clarify: We don't want to promise anything here, so something like

Specifying a zero duration on Unix platforms will return immediately

should not be there. That information would be fine if it was clear this is not a promise, but just a description of the current implementation. So, something like "Currently, .." or "The current implementation ...". See the documentation of SystemTime::now for an example like that.

Also, just like SystemTime::now's documentation, it'd be better to simply document which syscall/API it uses (e.g. Sleep) and link to its official documentation, instead of describing what that syscall does here (yielding the time slice). Then there's no risk of us getting the details of the Sleep(0) behaviour wrong, leaving the responsibility to Microsoft's own documentation. (It's fine to just do this for Windows now. We can always add a full table with the other platforms later.)

I'd say linking yield_now would still be good, as that's one of the reasons people might want to use a zero sleep. However, I believe Sleep(0) is slightly different than what yield_now does (SwitchToThread()). So we should be a bit careful in describing it as the alternative. So, something like "you might want to use .." instead of just "use .." would be better here.

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-9 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
   Compiling miniz_oxide v0.4.0
   Compiling hashbrown v0.9.0
   Compiling object v0.22.0
   Compiling addr2line v0.14.0
error[E0585]: found a documentation comment that doesn't document anything
    |
777 | ///
    | ^^^
    |
    |
    = help: doc comments must come before what they document, maybe a comment was intended with `//`?
error[E0432]: unresolved import `crate::thread::Result`
  --> library/std/src/panic.rs:18:5
   |
18 | use crate::thread::Result;
18 | use crate::thread::Result;
   |     ^^^^^^^^^^^^^^^^^^^^^ no `Result` in `thread`
error[E0432]: unresolved import `crate::thread::Thread`
 --> library/std/src/sync/mpsc/blocking.rs:6:27
  |
  |
6 | use crate::thread::{self, Thread};
  |                           ^^^^^^ no `Thread` in `thread`
error[E0432]: unresolved import `crate::thread::Thread`
  --> library/std/src/sync/once.rs:94:27
   |
   |
94 | use crate::thread::{self, Thread};
   |                           ^^^^^^ no `Thread` in `thread`
error[E0432]: unresolved import `crate::thread::Thread`
 --> library/std/src/sys_common/thread_info.rs:5:5
  |
5 | use crate::thread::Thread;
5 | use crate::thread::Thread;
  |     ^^^^^^^^^^^^^^^^^^^^^ no `Thread` in `thread`
error[E0432]: unresolved import `crate::thread::Thread`
  --> library/std/src/rt.rs:32:9
   |
32 |     use crate::thread::Thread;
32 |     use crate::thread::Thread;
   |         ^^^^^^^^^^^^^^^^^^^^^ no `Thread` in `thread`
error[E0432]: unresolved import `crate::thread::JoinHandle`
 --> library/std/src/sys/unix/ext/thread.rs:8:5
  |
8 | use crate::thread::JoinHandle;
8 | use crate::thread::JoinHandle;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^ no `JoinHandle` in `thread`
error: cannot find macro `thread_local` in this scope
   --> library/std/src/panicking.rs:237:5
    |
    |
237 |     thread_local! { static LOCAL_PANIC_COUNT: Cell<usize> = Cell::new(0) }

error: cannot find macro `thread_local` in this scope
  --> library/std/src/sys_common/thread_info.rs:12:1
   |
   |
12 | thread_local! { static THREAD_INFO: RefCell<Option<ThreadInfo>> = RefCell::new(None) }

error: cannot find macro `thread_local` in this scope
  --> library/std/src/io/stdio.rs:21:1
   |
   |
21 | thread_local! {

error: cannot find macro `thread_local` in this scope
    --> library/std/src/collections/hash/map.rs:2806:9
     |
     |
2806 |         thread_local!(static KEYS: Cell<(u64, u64)> = {


error[E0425]: cannot find value `KEYS` in this scope
     |
     |
2810 |         KEYS.with(|keys| {


error[E0425]: cannot find value `OUTPUT_CAPTURE` in this scope
    |
    |
907 |     OUTPUT_CAPTURE.with(move |slot| slot.replace(sink))


error[E0425]: cannot find value `OUTPUT_CAPTURE` in this scope
    |
    |
925 |         && OUTPUT_CAPTURE.try_with(|s| {


error[E0425]: cannot find function `current` in module `thread`
  --> library/std/src/sync/mpsc/blocking.rs:31:50
   |
31 |     let inner = Arc::new(Inner { thread: thread::current(), woken: AtomicBool::new(false) });
   |                                                  ^^^^^^^ not found in `thread`
help: consider importing this function
   |
   |
3  | use crate::sys::thread::guard::current;


error[E0425]: cannot find function `park` in module `thread`
  --> library/std/src/sync/mpsc/blocking.rs:68:21
68 |             thread::park()
   |                     ^^^^ not found in `thread`


error[E0425]: cannot find function `park_timeout` in module `thread`
  --> library/std/src/sync/mpsc/blocking.rs:79:21
   |
79 |             thread::park_timeout(end - now)
   |                     ^^^^^^^^^^^^ not found in `thread`

error[E0425]: cannot find function `yield_now` in module `thread`
   --> library/std/src/sync/mpsc/shared.rs:194:63
    |
194 | ...                   mpsc::Inconsistent => thread::yield_now(),
    |                                                     ^^^^^^^^^ not found in `thread`

error[E0425]: cannot find function `yield_now` in module `thread`
   --> library/std/src/sync/mpsc/shared.rs:298:29
298 |                     thread::yield_now();
    |                             ^^^^^^^^^ not found in `thread`


error[E0425]: cannot find function `yield_now` in module `thread`
   --> library/std/src/sync/mpsc/shared.rs:470:29
470 |                     thread::yield_now();
    |                             ^^^^^^^^^ not found in `thread`


error[E0425]: cannot find function `yield_now` in module `thread`
   --> library/std/src/sync/mpsc/stream.rs:416:29
416 |                     thread::yield_now();
    |                             ^^^^^^^^^ not found in `thread`


error[E0425]: cannot find function `current` in module `thread`
    |
    |
448 |             thread: Cell::new(Some(thread::current())),
    |                                            ^^^^^^^ not found in `thread`
help: consider importing this function
    |
    |
90  | use crate::sys::thread::guard::current;


error[E0425]: cannot find function `park` in module `thread`
    |
477 |             thread::park();
    |                     ^^^^ not found in `thread`


error[E0425]: cannot find function `panicking` in module `thread`
   |
   |
31 |         let ret = Guard { panicking: thread::panicking() };
   |                                              ^^^^^^^^^ not found in `thread`
help: consider importing this function
   |
1  | use crate::panicking::panicking;
   |
   |

error[E0425]: cannot find function `panicking` in module `thread`
   |
   |
37 |         if !guard.panicking && thread::panicking() {
   |                                        ^^^^^^^^^ not found in `thread`
help: consider importing this function
   |
1  | use crate::panicking::panicking;
   |
   |

error[E0425]: cannot find value `THREAD_INFO` in this scope
   |
19 |         THREAD_INFO
   |         ^^^^^^^^^^^ not found in this scope


error[E0425]: cannot find value `THREAD_INFO` in this scope
   |
   |
40 |     THREAD_INFO.with(|c| assert!(c.borrow().is_none()));


error[E0425]: cannot find value `THREAD_INFO` in this scope
   |
   |
41 |     THREAD_INFO.with(move |c| *c.borrow_mut() = Some(ThreadInfo { stack_guard, thread }));


error[E0425]: cannot find value `THREAD_INFO` in this scope
   |
   |
45 |     THREAD_INFO.with(move |c| c.borrow_mut().as_mut().unwrap().stack_guard = stack_guard);


error[E0425]: cannot find function `current` in module `thread`
  --> library/std/src/sys_common/util.rs:26:17
   |
26 |         thread::current().name().unwrap_or("<unknown>")
   |                 ^^^^^^^ not found in `thread`
help: consider importing this function
   |
   |
1  | use crate::sys::thread::guard::current;


error[E0425]: cannot find function `panicking` in module `thread`
    |
115 |     if thread::panicking() {
    |                ^^^^^^^^^ not found in `thread`


error[E0425]: cannot find function `panicking` in module `thread`
    |
163 |     if thread::panicking() {
    |                ^^^^^^^^^ not found in `thread`


error[E0425]: cannot find value `LOCAL_PANIC_COUNT` in this scope
    |
244 |     static GLOBAL_PANIC_COUNT: AtomicUsize = AtomicUsize::new(0);
244 |     static GLOBAL_PANIC_COUNT: AtomicUsize = AtomicUsize::new(0);
    |     ------------------------------------------------------------- similarly named static `GLOBAL_PANIC_COUNT` defined here
...
248 |         LOCAL_PANIC_COUNT.with(|c| {
    |         ^^^^^^^^^^^^^^^^^ help: a static with a similar name exists: `GLOBAL_PANIC_COUNT`

error[E0425]: cannot find value `LOCAL_PANIC_COUNT` in this scope
    |
244 |     static GLOBAL_PANIC_COUNT: AtomicUsize = AtomicUsize::new(0);
244 |     static GLOBAL_PANIC_COUNT: AtomicUsize = AtomicUsize::new(0);
    |     ------------------------------------------------------------- similarly named static `GLOBAL_PANIC_COUNT` defined here
...
257 |         LOCAL_PANIC_COUNT.with(|c| {
    |         ^^^^^^^^^^^^^^^^^ help: a static with a similar name exists: `GLOBAL_PANIC_COUNT`

error[E0425]: cannot find value `LOCAL_PANIC_COUNT` in this scope
    |
244 |     static GLOBAL_PANIC_COUNT: AtomicUsize = AtomicUsize::new(0);
244 |     static GLOBAL_PANIC_COUNT: AtomicUsize = AtomicUsize::new(0);
    |     ------------------------------------------------------------- similarly named static `GLOBAL_PANIC_COUNT` defined here
...
265 |         LOCAL_PANIC_COUNT.with(|c| c.get())
    |         ^^^^^^^^^^^^^^^^^ help: a static with a similar name exists: `GLOBAL_PANIC_COUNT`

error[E0425]: cannot find value `LOCAL_PANIC_COUNT` in this scope
    |
244 |     static GLOBAL_PANIC_COUNT: AtomicUsize = AtomicUsize::new(0);
244 |     static GLOBAL_PANIC_COUNT: AtomicUsize = AtomicUsize::new(0);
    |     ------------------------------------------------------------- similarly named static `GLOBAL_PANIC_COUNT` defined here
...
291 |         LOCAL_PANIC_COUNT.with(|c| c.get() == 0)
    |         ^^^^^^^^^^^^^^^^^ help: a static with a similar name exists: `GLOBAL_PANIC_COUNT`
error: unused import: `crate::cell::Cell`
 --> library/std/src/collections/hash/map.rs:9:5
  |
9 | use crate::cell::Cell;
9 | use crate::cell::Cell;
  |     ^^^^^^^^^^^^^^^^^
  |
  = note: `-D unused-imports` implied by `-D warnings`
error: unused import: `crate::sys`
  --> library/std/src/collections/hash/map.rs:16:5
   |
16 | use crate::sys;
16 | use crate::sys;
   |     ^^^^^^^^^^

error: unused import: `Cell`
  |
  |
8 | use crate::cell::{Cell, RefCell};

error: unused import: `crate::cell::RefCell`
 --> library/std/src/sys_common/thread_info.rs:3:5
  |
---
    |
233 |     use crate::cell::Cell;
    |         ^^^^^^^^^^^^^^^^^

error: unused import: `AsInner`
 --> library/std/src/sys/unix/ext/thread.rs:7:25
  |
7 | use crate::sys_common::{AsInner, IntoInner};

error: unused import: `IntoInner`
 --> library/std/src/sys/unix/ext/thread.rs:7:34
  |
  |
7 | use crate::sys_common::{AsInner, IntoInner};

error: aborting due to 43 previous errors

Some errors have detailed explanations: E0425, E0432, E0585.

Updated to specify the underlying syscalls
@rust-log-analyzer

This comment has been minimized.

Fix link reference

Co-authored-by: Joshua Nelson <[email protected]>
@JohnCSimon JohnCSimon removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 8, 2021
@JohnCSimon JohnCSimon added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 8, 2021
@Dylan-DPC-zz
Copy link

@Digital-Chaos any updates on changing this to match the meeting decision

@Digital-Chaos
Copy link
Contributor Author

Digital-Chaos commented Feb 9, 2021

@Digital-Chaos any updates on changing this to match the meeting decision

@Dylan-DPC, I updated the PR on 15th Jan with more info taking into account the feedback from @m-ou-se, and which was subsequently fixed by @jyn514 (also the 15th Jan), not quite sure why the status this PR is tagged as S-waiting-on-author?

@jyn514
Copy link
Member

jyn514 commented Feb 9, 2021

Ah ok, I didn't realize this was waiting on review. In the future you can use

@rustbot modify labels: -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 9, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Feb 9, 2021

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 9, 2021

📌 Commit bb2a27b has been approved by m-ou-se

@bors 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 Feb 9, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 10, 2021
Rollup of 11 pull requests

Successful merges:

 - rust-lang#79849 (Clarify docs regarding sleep of zero duration)
 - rust-lang#80438 (Add `Box::into_inner`.)
 - rust-lang#81466 (Add suggest mut method for loop)
 - rust-lang#81687 (Make Vec::split_at_spare_mut public)
 - rust-lang#81904 (Bump stabilization version for const int methods)
 - rust-lang#81909 ([compiler/rustc_typeck/src/check/expr.rs] Remove unnecessary refs in pattern matching)
 - rust-lang#81910 (Use format string in bootstrap panic instead of a string directly)
 - rust-lang#81913 (Rename HIR UnOp variants)
 - rust-lang#81925 (Add long explanation for E0547)
 - rust-lang#81926 (add suggestion to use the `async_recursion` crate)
 - rust-lang#81951 (Update cargo)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit bb06b13 into rust-lang:master Feb 10, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.