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.

Sign up for GitHub

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

Jump to bottom

RWLock: Add deadlock example #82624

Merged
merged 1 commit into from
Jun 28, 2021
Merged

RWLock: Add deadlock example #82624

merged 1 commit into from
Jun 28, 2021

Conversation

ojeda
Copy link
Contributor

@ojeda ojeda commented Feb 28, 2021

Suggested in #82596 but it was a bit too late.

@matklad @azdavis @sfackler

@rust-highfive
Copy link
Collaborator

r? @sfackler

(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 Feb 28, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@ojeda ojeda force-pushed the rwlock-example-deadlock branch from 78d28ca to c1526ab Compare February 28, 2021 08:42
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@ojeda ojeda force-pushed the rwlock-example-deadlock branch from c1526ab to 3a72c32 Compare February 28, 2021 11:31
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@ojeda ojeda force-pushed the rwlock-example-deadlock branch from 3a72c32 to 0648a88 Compare February 28, 2021 12:23
sfackler
sfackler reviewed Feb 28, 2021
View reviewed changes
library/std/src/sync/rwlock.rs
/// `read`.
/// `read`, e.g.:
///
/// ```text
Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced we spend a bunch of vertical space on a fairly obscure edge case, but I'd defer to @rust-lang/docs.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's up to you and libs; we don't have any real precedent here in either direction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps we can get the best of both worlds: is there a way to collapse a subset of the docs by default? It would be useful for detailed examples, e.g. like we do in GitHub:

Time-threads diagram 
// Thread 1             |  // Thread 2
let _rg = lock.read();  |
                        |  // will block
                        |  let _wg = lock.write();
// may deadlock         |
let _rg = lock.read();  |

Copy link
Member

Choose a reason for hiding this comment

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

This is with the <details> tag, which, given that markdown is a superset of HTML, should be usable. I don't think we currently use it at all, though.

Copy link
Contributor Author

@ojeda ojeda Mar 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Tested it, and it works nicely, although it needs a small tweak: see #82805.

From what I can tell, at least we use <details> in the unstable/yellow orange boxes.

ojeda added a commit to ojeda/rust that referenced this pull request Mar 5, 2021
@ojeda
rustdoc: CSS: show the <details> "triangle" in all cases
5dccb4e 
Currently we use `<details>` and `<summary>` for the unstable
info box, under class `.stab`. However, they can also be useful
to add detailed documentation like lengthy detailed examples
that would be too much to show by default, e.g.:

    /// <details><summary>Time-threads diagram</summary>
    ///
    /// ```text
    /// // Thread 1             |  // Thread 2
    /// let _rg = lock.read();  |
    ///                         |  // will block
    ///                         |  let _wg = lock.write();
    /// // may deadlock         |
    /// let _rg = lock.read();  |
    /// ```
    /// </details>

See rust-lang#82624 for an example
where we could use this functionality.

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
@ojeda ojeda mentioned this pull request Mar 5, 2021
Closed
@ojeda
RWLock: Add deadlock example
98096a9 
Suggested in rust-lang#82596 but it was
a bit too late.

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
@ojeda ojeda force-pushed the rwlock-example-deadlock branch from 0648a88 to 98096a9 Compare March 5, 2021 17:14
@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 Mar 21, 2021
@Dylan-DPC-zz
Copy link

@sfackler this is ready for review

@ojeda
Copy link
Contributor Author

ojeda commented Mar 22, 2021

I need to finish #82805 first, then we can merge this, sorry.

@Dylan-DPC-zz
Copy link

Dylan-DPC-zz commented Mar 22, 2021 •
edited
Loading

@ojeda sure no worries. Will mark this as blocked till then :)

nevermind they are unrelated prs

@ojeda
Copy link
Contributor Author

ojeda commented Mar 22, 2021

nevermind they are unrelated prs

This PR requires #82805 to display properly the details HTML tag in the docs.

@Dylan-DPC-zz
Copy link

ah thanks for confirming. marking it as blocked

@Dylan-DPC-zz Dylan-DPC-zz added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 22, 2021
@JohnTitor JohnTitor added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jun 28, 2021
JohnTitor
JohnTitor approved these changes Jun 28, 2021
View reviewed changes
Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

#86589 which re-opened #82805 has been merged and the example itself looks good to me. About the style issue, we can come back here if someone issues it.

@JohnTitor

This comment has been minimized.

Sign in to view
@rust-highfive rust-highfive assigned JohnTitor and unassigned sfackler Jun 28, 2021
@JohnTitor

This comment has been minimized.

Sign in to view
@JohnTitor JohnTitor closed this Jun 28, 2021
@JohnTitor JohnTitor reopened this Jun 28, 2021
@JohnTitor
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 28, 2021

📌 Commit 98096a9 has been approved by JohnTitor

@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 28, 2021
@bors
Copy link
Contributor

bors commented Jun 28, 2021

⌛ Testing commit 98096a9 with merge 17ea490...

@bors
Copy link
Contributor

bors commented Jun 28, 2021

☀️ Test successful - checks-actions
Approved by: JohnTitor
Pushing 17ea490 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 28, 2021
@bors bors merged commit 17ea490 into rust-lang:master Jun 28, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 28, 2021
@ojeda ojeda deleted the rwlock-example-deadlock branch November 15, 2021 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steveklabnik steveklabnik steveklabnik left review comments

@sfackler sfackler sfackler left review comments

@JohnTitor JohnTitor JohnTitor approved these changes

Assignees

@JohnTitor JohnTitor

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.55.0
Development

Successfully merging this pull request may close these issues.
10 participants
@ojeda @rust-highfive @rust-log-analyzer @Dylan-DPC-zz @JohnTitor @bors @steveklabnik @sfackler @JohnCSimon @rustbot