Revamped "Threads" documentation #21152
Conversation
| @@ -0,0 +1,343 @@ | |||
| % Concurrency | |||
|
|
|||
| Concurrency and paralellism are incredibly important topics in computer | |||
There was a problem hiding this comment.
Sigh, I thought I had spellcheck on, but I guess not. Thanks for all the saves here.
|
I need to rebase this, and #2080 should be addressed too. |
steveklabnik
force-pushed
the
tasks
branch
2 times, most recently
from
1ecc754 to
ac4581f
Compare
|
Rebased and fixed up. @aturon , r? |
| ## Background: `Send` and `Sync` | ||
|
|
||
| Concurrency is difficult to reason about. In Rust, we have a strong, static type | ||
| system to help us reason about our code. As such, Rust gives us two types to help |
There was a problem hiding this comment.
Note that Send and Sync are traits, not types -- they are markers that can be applied to other types.
There was a problem hiding this comment.
You may also want to say something about the fact that Send and Sync are automatically applied to types.
|
@aturon I tried to address all of your points, so this is ready for another review :) |
| ``` | ||
|
|
||
| First, we call `lock()`, which aquires the mutex's lock. Because this may fail, | ||
| it returns an `Option<T>`, and because this is just an example, we `unwrap()` |
There was a problem hiding this comment.
lock actually returns a Result, and it's maybe worth saying that it fails if a thread previously holding the lock panicked.
|
cc @sfackler @pythonesque -- I gave @steveklabnik an initial round of feedback, but there's still plenty of room for improvement here. I'm ok to land this as-is and improve though follow-up PRs, but if either of you have some time to help @steveklabnik iterate on it before landing, that would be great. Some possible further improvements:
|
| use std::time::Duration; | ||
|
|
||
| fn main() { | ||
| let data = Arc::new(Mutex::new(0u32)); |
There was a problem hiding this comment.
It's a bit confusing that the type of data is changed from Vec<u32> to u32 here. The code should be consistent with previous examples.
Also if you want to keep Vec<u32>, the range must be written as 0us .. 2 otherwise you get the following error:
error: the trait `core::ops::IndexMut<i32>` is not implemented
for the type `collections::vec::Vec<u32>` [E0277]
Typos asides, nice work! :)
There was a problem hiding this comment.
+1 for consistently with earlier examples.
I was also confused as to why, when the doc zeroes in on marker::NoSend as the big problem, the next sentence presents Arc as the solution. It seems to me like the proximal solution was to move data.lock().unwrap() inside the thread, so that the guard is not sent into the closure. I guess Arc is somehow necessary to move that line inside the closure, but the mechanism isn't clear.
I think the pedagogical approach of showing a series of failed attempts is very useful. Perhaps this section could benefit from adding yet another failed attempt to show the problem with moving data.lock.unwrap() inside the closure without Arc. After that we would see the final Arc example. That structure would be more precise about the problem Arc solves.
There was a problem hiding this comment.
Yes, this could probably be improved. In the interest of actually landing this, I think doing that is worth waiting for, when we can address things like #21152 (comment)
There was a problem hiding this comment.
(the consistency part will be improved, I meant more the NoSend bit
| ``` | ||
|
|
||
| We now call `clone()` on our `Arc`, which increases the reference count. This | ||
| reference is then moved into the new thread. Let's examine the body of the |
There was a problem hiding this comment.
It may be confusing to use reference here, since a "reference" is & in Rust.
There was a problem hiding this comment.
switched to 'internal,' what do you think?
There was a problem hiding this comment.
I don't quite understand what you mean. ("This internal is then moved into the ..."??)
There was a problem hiding this comment.
Oh, i edied the previous line, 'increase the internal count' 😓
There was a problem hiding this comment.
we went with 'handle'
|
@huonw thanks for the feedback. i incorporated it all 👍 |
steveklabnik
force-pushed
the
tasks
branch
2 times, most recently
from
52f47d6 to
87da44a
Compare
|
(BTW, basically r=me after my last look at this, so send it on to bors once @huonw is happy) |
|
@bors r+ 87da |
|
|
||
| Specifically, `F`, the closure that we pass to execute in the new thread. It | ||
| has two restrictions: It must be a `FnOnce` from `()` to `T`. Using `FnOnce` | ||
| allows the clousre to take ownership of any data it mentions from the parent |
|
⌛ Testing commit 87da44a with merge 852a2f2... |
|
💔 Test failed - auto-win-32-nopt-t |
|
I don't know how to fix this error, any ideas? |
| fn main() { | ||
| let data = Arc::new(Mutex::new(vec![1u32, 2, 3])); | ||
|
|
||
| for i in (0us..10) { |
There was a problem hiding this comment.
I this supposed to iterate up to 10? The length of the array is only 3. (this is causing the panic in the doc test and then windows may not be dealing with the panic-during-shutdown and aborting the process).
There was a problem hiding this comment.
i wonder why i didn't catch this when i ran my own tests
|
I have fixed this locally but this wifi blocks the |
Fixes rust-lang#18936 Fixes rust-lang#18938 Fixes rust-lang#20038 Fixes rust-lang#8395 Fixes rust-lang#2080 Fixes rust-lang#21194
This moves the "Tasks" chapter to a "Concurrency" one, as it's about threads, but also about how to deal with concurrency issues. r? @aturon
|
@steveklabnik for future reference, this may help push things to GitHub around SSH blocks: https://help.github.com/articles/using-ssh-over-the-https-port/ |
|
Thanks Tom! It turns out that because of TFA, to push over ssh i needed to
use a token, not my password...
|
This moves the "Tasks" chapter to a "Concurrency" one, as it's about threads, but also about how to deal with concurrency issues.
r? @aturon