Don't encourage people to ignore threading errors in the docs #44962

shepmaster · 2017-10-01T18:56:37Z

No description provided.

rust-highfive · 2017-10-01T18:56:48Z

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

sfackler · 2017-10-01T19:29:57Z

src/libstd/thread/mod.rs

@@ -1192,7 +1194,7 @@ impl<T> JoinInner<T> {
 ///     });
 /// });
 ///
-/// let _ = original_thread.join();
+/// original_thread.join().expect("Unable to join thread");


Join will return an error if the other thread panicked, so this error message may not be the most accurate.

Is it then correct to say that there two failure cases that should be discussed in each case?

The thread panicked.

We couldn't join (whatever underlying error).

The existing docs for JoinHandle::join say:

join_handle.join().expect("Couldn't join on the associated thread");

Would you like to see both of the above cases mentioned in every expect? Just in the example for JoinHandle::join and hope people read it? Something else?

I always regret adding unwrap to examples because that teaches people to not have useful tracer messages, but saying nothing useful avoids saying something wrong.

I would think the JoinHandle docs should change as well.

I would think the JoinHandle docs should change as well.

To be clear, you'd like all the examples to have something like this?

.join().expect("Unable to join thread or thread panicked")

Potentially, though the contexts in which the error was due to a join error are really limited.

This is personally why I use unwrap over expect; it's tough to get a good message.

@sfackler , if you could be more concrete here, that'd be great. I'm happy with whatever, but right now, it's unclear what exactly @shepmaster needs to do for this to land.

This is personally why I use unwrap over expect; it's tough to get a good message.

Wait what?! 😨 It's always better to have a message than just nothing!

@shepmaster Docs indicate that the only error that .join() can return is if the thread has panicked

@bluss good point; checking the implementation seems to bear that out:

impl<T> JoinInner<T> { fn join(&mut self) -> Result<T> { self.native.take().unwrap().join(); unsafe { (*self.packet.0.get()).take().unwrap() } } }

The actual join has no potential Result. Based on that, it seems that the correct course of action is that the messages should say something akin to expect("the thread being joined has panicked").

sfackler · 2017-10-01T23:26:52Z

@rust-lang/docs

GuillaumeGomez · 2017-10-02T10:01:07Z

I think it's all good except for the issue you pointed at. Once done, it's all good for me.

steveklabnik · 2017-10-05T20:57:13Z

@bors: r+ rollup

thanks!

bors · 2017-10-05T20:57:13Z

📌 Commit 928013b has been approved by steveklabnik

GuillaumeGomez · 2017-10-06T08:47:44Z

Sorry @steveklabnik, I just saw a lint so I'll r- for now. ;)

@bors: r-

GuillaumeGomez · 2017-10-06T08:48:20Z

src/libstd/thread/mod.rs

@@ -485,15 +485,17 @@ impl Builder {
 /// let (tx, rx) = channel();
 ///
 /// let sender = thread::spawn(move || {
-///     let _ = tx.send("Hello, thread".to_owned());
+///     tx.send("Hello, thread".to_owned())
+///         .expect("Unable to send on channel");


Invalid indent, should be:

tx.send("Hello, thread".to_owned()) .expect("Unable to send on channel");

Invalid indent

Huh. Rustfmt is what indented it this way. Where is this style documented so I can check it before submitting future PRs?

rustfmt is really bad... Well, whatever, please align the methods' calls.

Where is this style documented so I can check it before submitting future PRs?

No clue. It's how it's done in the rest of rustc source code. Sorry but I have no idea if there is such a document...

bors · 2017-10-08T14:27:30Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: MIR-borrowck: gather and signal any move errors #45016

bors · 2017-10-08T14:27:30Z

📌 Commit 37724f8 has been approved by steveklabnik

steveklabnik · 2017-10-08T14:31:35Z

@bors: r+

This is in fact standard style, which rustc has not yet fully adopted, but will. We want to show standard style in the docs. I can understand if you don't like that style, @GuillaumeGomez , as there's some parts I'm not psyched about either, but sticking with the standard is worth arguing over quibbles like these.

bors · 2017-10-08T14:31:36Z

📌 Commit b5b7666 has been approved by steveklabnik

GuillaumeGomez · 2017-10-09T08:11:50Z

My bad, thought it was the standard style.

…eklabnik Don't encourage people to ignore threading errors in the docs

Rollup of 9 pull requests - Successful merges: #44962, #45051, #45091, #45106, #45117, #45118, #45120, #45125, #45136 - Failed merges:

rust-highfive assigned sfackler Oct 1, 2017

sfackler reviewed Oct 1, 2017

View reviewed changes

shepmaster force-pushed the no-ignore-result branch from 0b4412c to 928013b Compare October 3, 2017 00:41

GuillaumeGomez reviewed Oct 6, 2017

View reviewed changes

shepmaster force-pushed the no-ignore-result branch from 928013b to 37724f8 Compare October 7, 2017 17:51

rust-lang deleted a comment from bors Oct 8, 2017

Don't encourage people to ignore threading errors in the docs

b5b7666

shepmaster force-pushed the no-ignore-result branch from 37724f8 to b5b7666 Compare October 8, 2017 14:29

kennytm mentioned this pull request Oct 9, 2017

Rollup of 9 pull requests #45141

Merged

kennytm added a commit to kennytm/rust that referenced this pull request Oct 9, 2017

Rollup merge of rust-lang#44962 - shepmaster:no-ignore-result, r=stev…

dad731c

…eklabnik Don't encourage people to ignore threading errors in the docs

bors added a commit that referenced this pull request Oct 10, 2017

Auto merge of #45141 - kennytm:rollup, r=kennytm

5f578df

Rollup of 9 pull requests - Successful merges: #44962, #45051, #45091, #45106, #45117, #45118, #45120, #45125, #45136 - Failed merges:

bors merged commit b5b7666 into rust-lang:master Oct 10, 2017

shepmaster deleted the no-ignore-result branch October 10, 2017 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't encourage people to ignore threading errors in the docs #44962

Don't encourage people to ignore threading errors in the docs #44962

shepmaster commented Oct 1, 2017

rust-highfive commented Oct 1, 2017

sfackler Oct 1, 2017

shepmaster Oct 1, 2017

sfackler Oct 1, 2017 •

edited

Loading

shepmaster Oct 1, 2017

sfackler Oct 1, 2017

steveklabnik Oct 2, 2017

GuillaumeGomez Oct 2, 2017

bluss Oct 2, 2017

shepmaster Oct 3, 2017

sfackler commented Oct 1, 2017

GuillaumeGomez commented Oct 2, 2017

steveklabnik commented Oct 5, 2017

bors commented Oct 5, 2017

GuillaumeGomez commented Oct 6, 2017 •

edited

Loading

GuillaumeGomez Oct 6, 2017

shepmaster Oct 6, 2017

GuillaumeGomez Oct 7, 2017

shepmaster Oct 7, 2017

GuillaumeGomez Oct 8, 2017

bors commented Oct 8, 2017

bors commented Oct 8, 2017

steveklabnik commented Oct 8, 2017

bors commented Oct 8, 2017

GuillaumeGomez commented Oct 9, 2017

Don't encourage people to ignore threading errors in the docs #44962

Don't encourage people to ignore threading errors in the docs #44962

Conversation

shepmaster commented Oct 1, 2017

rust-highfive commented Oct 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfackler Oct 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfackler commented Oct 1, 2017

GuillaumeGomez commented Oct 2, 2017

steveklabnik commented Oct 5, 2017

bors commented Oct 5, 2017

GuillaumeGomez commented Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Oct 8, 2017

bors commented Oct 8, 2017

steveklabnik commented Oct 8, 2017

bors commented Oct 8, 2017

GuillaumeGomez commented Oct 9, 2017

sfackler Oct 1, 2017 •

edited

Loading

GuillaumeGomez commented Oct 6, 2017 •

edited

Loading