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

Explain non-dropped sender recv in docs #80269

Merged
merged 1 commit into from
Jun 15, 2021
Merged

Conversation

pickfire
Copy link
Contributor

@pickfire pickfire commented Dec 21, 2020

Original senders that are still hanging around could cause
Receiver::recv to not block since this is a potential footgun
for beginners, clarify more on this in the docs for readers to
be aware about it.

Maybe it would be better to show an example of the pattern where drop(tx) is used when it is being cloned multiple times? Although I have seen it in quite a few articles but I am surprised that this part is not very clear with the current words without careful reading.

If the corresponding Sender has disconnected, or it disconnects while this call is blocking, this call will wake up and return Err to indicate that no more messages can ever be received on this channel. However, since channels are buffered, messages sent before the disconnect will still be properly received.

Some words there may seemed similar if I carefully read and relate it but if I am new, I probably does not know "drop" makes it "disconnected". So I mention the words "drop" and "alive" to make it more relatable to lifetime.

@rust-highfive
Copy link
Collaborator

r? @dtolnay

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 21, 2020
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 11, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 1, 2021
@Dylan-DPC-zz
Copy link

r? @KodrAus

@rust-highfive rust-highfive assigned KodrAus and unassigned dtolnay Feb 17, 2021
@KodrAus
Copy link
Contributor

KodrAus commented Feb 19, 2021

Thanks for the PR @pickfire!

That original wording is a little unclear that it won’t unblock unless all senders are dropped, isn’t it.

Maybe we could just clarify it a bit more there and avoid sprinkling too many more warnings around?

@pickfire
Copy link
Contributor Author

That original wording is a little unclear that it won’t unblock unless all senders are dropped, isn’t it.

Where is that? I can't seemed to find it. I did try finding that but I couldn't find anything so I wrote this PR.

Maybe we could just clarify it a bit more there and avoid sprinkling too many more warnings around?

I searched though the file but I can't see it and tried reading the docs again. Can you please point me to the exact line and word you are talking about?

@KodrAus
Copy link
Contributor

KodrAus commented Feb 21, 2021

@pickfire That's in the docs for the Receiver::recv method.

Having another look through our docs, maybe the section on disconnection in the mpsc module root could also use a clarification that dropping senders means that all instances of it need to be dropped.

What do you think?

@pickfire
Copy link
Contributor Author

pickfire commented Feb 22, 2021

@pickfire That's in the docs for the Receiver::recv method.

Where? I read that part, I had even modified that part in this pull request and yet I didn't saw it. I think this part is quite blur and did not mention the word "drop". If you can point me the exact part out maybe I can switch the keywords to "drop" so it is simpler to understand. Would that be better?

Having another look through our docs, maybe the section on disconnection in the mpsc module root could also use a clarification that dropping senders means that all instances of it need to be dropped.

Adding it to both recv and disconnection would be best. Even better if we show an example of the use case of dropping the sender on the examples section below disconnection.

cc @jyn514 for https://github.com/rust-lang/rust/pull/81833/files#r575693721

@jyn514
Copy link
Member

jyn514 commented Feb 22, 2021

@pickfire why did you ping me? I don't understand what #81833 has to do with this PR.

@pnkfelix
Copy link
Member

pnkfelix commented Feb 22, 2021

@jyn514 I'm guessing it was because of the conversation on a review comment there that was subsequently resolved (and thus is remarkably hard to link to directly:

Click to see screen shot of conversation

image

@pickfire
Copy link
Contributor Author

@jyn514 Do you see the drop(tx) pattern being explained in the existing docs? If you don't as well, I guess really the current docs is missing this part. Or wait, I think can see it (and relate it) now

This function will always block the current thread if there is no data available and it's possible for more data to be sent.

"it's possible for more data to be sent." but I think not everyone can easily relate that as "all senders have been dropped".
Is it this one?

@jyn514
Copy link
Member

jyn514 commented Feb 24, 2021

Personally, I don't think any documentation changes would have made me not ask questions in #81833 - I'm very unfamiliar with actor-style programming and I wouldn't have thought to look at the API reference for a conceptual overview. I don't think this is a bad change, I just don't have any feedback to give.

@crlf0710 crlf0710 added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 12, 2021
@the8472
Copy link
Member

the8472 commented Mar 12, 2021

Disconnection itself is explained in prose, which code patterns arise from that isn't reflected in the examples since they only use single steps or counted loops. The importance of correct ordering in the drop(tx); thread_guard.join() pattern stems from using channels with unbounded receive loops.

Copy link

@Dylan-DPC-zz Dylan-DPC-zz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor tidbits

library/std/src/sync/mpsc/mod.rs Outdated Show resolved Hide resolved
library/std/src/sync/mpsc/mod.rs Outdated Show resolved Hide resolved
library/std/src/sync/mpsc/mod.rs Outdated Show resolved Hide resolved
library/std/src/sync/mpsc/mod.rs Outdated Show resolved Hide resolved
library/std/src/sync/mpsc/mod.rs Outdated Show resolved Hide resolved
library/std/src/sync/mpsc/mod.rs Outdated Show resolved Hide resolved
library/std/src/sync/mpsc/mod.rs Outdated Show resolved Hide resolved
@pickfire
Copy link
Contributor Author

Disconnection itself is explained in prose, which code patterns arise from that isn't reflected in the examples since they only use single steps or counted loops. The importance of correct ordering in the drop(tx); thread_guard.join() pattern stems from using channels with unbounded receive loops.

I think it would be better if we have an example of using drop to show the cases in which it is necessary for every last transmitter to be dropped. What do you think? Or maybe you already have an example in mind?

@the8472
Copy link
Member

the8472 commented Mar 14, 2021

I'm not sure how that's different from what I have said, for drop(tx) to have any effect of course all other senders also have to be dropped. I didn't have a very specific example in mind, just something that doesn't use a counted loop.

@pickfire
Copy link
Contributor Author

I added an example based on tokio drop to show the cases we need drop.

@rust-log-analyzer

This comment has been minimized.

@dtolnay dtolnay assigned joshtriplett and unassigned dtolnay Apr 21, 2021
@jyn514 jyn514 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 Apr 22, 2021
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 8, 2021
@JohnCSimon
Copy link
Member

ping from triage:
@pickfire - can you please address the change request from joshtriplett - or if you have already can you mark the change request as completed?

@pickfire
Copy link
Contributor Author

Yes, I have addressed the changes requested.

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 11, 2021
@JohnTitor
Copy link
Member

@pickfire Could you squash commits into one? I'll r=josh once it's done as per #80269 (comment).

Original senders that are still hanging around could cause
Receiver::recv to not block since this is a potential footgun
for beginners, clarify more on this in the docs for readers to
be aware about it.

Fix minor tidbits in sender recv doc

Co-authored-by: Dylan DPC <dylan.dpc@gmail.com>

Add example for unbounded receive loops in doc

Show the drop(tx) pattern, based on tokio docs
https://tokio-rs.github.io/tokio/doc/tokio/sync/index.html

Fix example code for drop sender recv

Fix wording in sender docs

Co-authored-by: Josh Triplett <josh@joshtriplett.org>
@pickfire
Copy link
Contributor Author

Done

@JohnTitor
Copy link
Member

Looks like you forgot to push it?

@pickfire
Copy link
Contributor Author

Ah, thanks. I missed that.

@JohnTitor
Copy link
Member

JohnTitor commented Jun 15, 2021

Thanks!

@bors r=joshtriplett rollup

@JohnTitor
Copy link
Member

Hmm, bors didn't catch it?
@bors r=joshtriplett

@Mark-Simulacrum
Copy link
Member

@bors r=joshtriplett rollup

@bors
Copy link
Contributor

bors commented Jun 15, 2021

📌 Commit 0f3c7d1 has been approved by joshtriplett

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

Successful merges:

 - rust-lang#80269 (Explain non-dropped sender recv in docs)
 - rust-lang#82179 (Add functions `Duration::try_from_secs_{f32, f64}`)
 - rust-lang#85608 (Stabilize `ops::ControlFlow` (just the type))
 - rust-lang#85792 (Refactor windows sockets impl methods)
 - rust-lang#86220 (Improve maybe_uninit_extra docs)
 - rust-lang#86277 (Remove must_use from ALLOWED_ATTRIBUTES)
 - rust-lang#86285 (:arrow_up: rust-analyzer)
 - rust-lang#86294 (Stabilize {std, core}::prelude::rust_*.)
 - rust-lang#86306 (Add mailmap entries for myself)
 - rust-lang#86314 (Remove trailing triple backticks in `mut_keyword` docs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2d2f1a5 into rust-lang:master Jun 15, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 15, 2021
@pickfire pickfire deleted the patch-4 branch June 16, 2021 03:28
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.