expand thread::park explanation

[RalfJung]

Cc @carllerche @parched @stjepang

[rust-highfive]

r? @Kimundi

(rust_highfive has picked a reviewer for you, use r? to override)

[RalfJung]

One thing I still dislike is that the example does not park in a loop. It is a bit too simplified to show the usual pattern. Fixing that, however, would require introducing some extra shared state to use as a signal (to make sure the wakeup wasn't spurious) -- is that worth it?

[ghost]

Fixing that, however, would require introducing some extra shared state to use as a signal (to make sure the wakeup wasn't spurious) -- is that worth it?

I'd add it - perhaps some kind of let ready = Arc::new(AtomicBool::new(false)) and then we do ready.store(true, Release); thread.unpark(), while the thread waits for the variable to become true by parking itself.

Btw, looks like we don't need the thread builder in the example - a simple thread::spawn() will do just fine. That would remove some cruft from the code.

[parched]

This looks fine to me, however, I'm wondering why we don't make the guarantee it will wait? The current implementation does and I can't really think of a sensible implementation that wouldn't. Even if you just implemented it with a raw FUTEX_WAIT, adding an extra CAS to stop a spurious return is negligible compared to the syscall itself.

[RalfJung]

According to @carllerche, it is a standard "litmus test" for correct usage of park/unpark to consider an implementation that just does nothing. I suppose this permits more efficient implementations, though I am not very knowledgeable in why the API was chosen to be the way it is.

@parched Are you essentially proposing to rule out spurious wakeups, or what else do you mean by "it will wait"?

[parched]

@parched Are you essentially proposing to rule out spurious wakeups

Yes exactly, because I don't think we gain anything by allowing it.

[RalfJung]

I think part of the answer is that most users wouldn't even benefit -- if you e.g. implement a lock on top of this, you anyway already have another channel to know if you can go on. park/unpark is an optimization over spinning, no more.

But I'll wait other people that know more about this APIs to fill us in here. ;)

[carllerche]

@parched Spurious notifications are permitted to enable more efficient implementations. IMO it would not be reasonable to rule them out.

park / unpark should never be used as part of the logic of the algorithm, just a way to block the thread. As such, there should always be some other variable used to determine if the unpark was "spurious".

[nagisa]

Other than the wording nit around the paragraph permitting spuriousness this looks good to me. I would like to see this reworded, but otherwise r=me.

[nagisa]

s/NOPs that return immediately/return immediately/.

[nagisa]

In general since this is a public API, I would prefer wording to not be in terms of how the implementation could be (i.e. implemented to return immediately), but what the implementation is allowed to do (return spuriously without necessarily synchronizing with anything).

[RalfJung]

I think examples are very useful to demonstrate where spurious wakeups may come from. For me personally, I have seen this "spurious wakeups allowed" stuff often, but only when I recently read "NOPs are a valid implementation" then it clicked for me.

[RalfJung]

I have changed the order a bit, putting the API contract first and the example second. What do you think?

[RalfJung]

@Kimundi @nagisa anything left to r+ this?

[nagisa]

@bors r+

…

On Sun, Dec 9, 2018, 21:25 Ralf Jung ***@***.*** wrote: @Kimundi <https://github.com/Kimundi> @nagisa <https://github.com/nagisa> anything left to r+ this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#56157 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AApc0vp0ess-i-QikkS967PwEUWytlOdks5u3WOigaJpZM4YvARK> .

[bors]

📌 Commit 76cd8f0 has been approved by nagisa

[bors]

⌛ Testing commit 76cd8f0 with merge c7c3310ff44af9f13e5f9e171f0a1ce6c6eb3c87...

[bors]

💔 Test failed - status-travis

[rust-highfive]

The job arm-android of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[01:28:35] test string::test_str_clear ... ok
[01:28:35] test string::test_str_truncate ... ok
[01:28:35] test string::test_str_truncate_invalid_len ... ok
[01:28:35] test string::test_str_truncate_split_codepoint ... ok
[01:28:36] died due to signal 11
[01:28:36] 
[01:28:36] 
[01:28:36] 
[01:28:36] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "arm-linux-androideabi" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "alloc" "--"
[01:28:36] 
[01:28:36] 
[01:28:36] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test --target arm-linux-androideabi
[01:28:36] Build completed unsuccessfully in 1:18:18
---
travis_time:end:0e74177e:start=1544431485745226518,finish=1544431485761930409,duration=16703891
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:01523340
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:03d257a0
travis_time:start:03d257a0
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0a35aede
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[RalfJung]

Looks spurious to me (the changed doctest passed on the PR CI, and the failure looks to be in a different crate anyway)

@bors retry

[bors]

⌛ Testing commit 76cd8f0 with merge 3a75e80...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: nagisa
Pushing 3a75e80 to master...