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

Tracking Issue for owned locked stdio handles #86845

Closed
2 of 4 tasks
tlyu opened this issue Jul 3, 2021 · 13 comments · Fixed by #93965
Closed
2 of 4 tasks

Tracking Issue for owned locked stdio handles #86845

tlyu opened this issue Jul 3, 2021 · 13 comments · Fixed by #93965
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@tlyu
Copy link
Contributor

tlyu commented Jul 3, 2021

Feature gate: #![feature(stdio_locked)]

This is a tracking issue for adding owned locked handles to stdio.

Especially for beginners, using stdio handles can involve intimidating problems with locking and lifetimes. First, the user has to call a free function (stderr(), stdin(), stdout()) to get a handle on the stream; then, the user might have to call the lock() method (for example, to gain access to the lines() iterator constructor). At this point, lifetime issues rapidly arise: the following code (playground) will produce a compile error.

use std::io;
fn main() {
    let _h = io::stdin().lock();
}
error[E0716]: temporary value dropped while borrowed
 --> src/main.rs:3:14
  |
3 |     let _h = io::stdin().lock();
  |              ^^^^^^^^^^^       - temporary value is freed at the end of this statement
  |              |
  |              creates a temporary which is freed while still in use
4 | }
  | - borrow might be used here, when `_h` is dropped and runs the destructor for type `StdinLock<'_>`
  |
  = note: consider using a `let` binding to create a longer lived value

The need to create a let binding to the handle seems confusing and frustrating, especially if the program does not need to use the handle again. The explanation is that the lock behaves as if it borrows the original handle from stdin(), and the temporary value created for the call to the lock() method is dropped at the end of the statement, invalidating the borrow. That explanation might be beyond the current level of understanding of a beginner who simply wants to write an interactive terminal program.

Even experienced Rust programmers sometimes have trouble with the lifetime management required for using locked stdio handles: (comment), (comment).

Fortunately, the stdio handles don't contain any non-static references, so it's possible to create owned locks such as StdinLock<'static>. This proposal creates methods and free functions for creating owned locked stdio handles. The methods consume self, eliminating any lifetime problems. The free functions directly return an owned locked handle, for programs where there is no need to use an unlocked handle. The implementation currently depends on the mutex references in the stdio handles being static, but this does not preclude future changes to the locking internals (for example, using Arc to wrap the mutex).

Public API

// std::io

pub fn stderr_locked() -> StderrLock<'static> { /* ... */ }
pub fn stdin_locked() -> StdinLock<'static> { /* ... */ }
pub fn stdout_locked() -> StdoutLock<'static> { /* ... */ }
impl Stderr {
    pub fn into_locked(self) -> StderrLock<'static> { /* ... */ }
}
impl Stdin {
    pub fn into_locked(self) -> StdinLock<'static> { /* ... */ }
}
impl Stdout {
    pub fn into_locked(self) -> StdoutLock<'static> { /* ... */ }
}

Steps / History

Unresolved Questions

  • During stabilization, might we want to change the documentation to encourage people to prefer the free functions that obtain owned locked handles (stdin_locked(), etc.) over the ones that obtain unlocked handles (stdin(), etc.)?

@rustbot label +A-io +D-newcomer-roadblock

@tlyu tlyu added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jul 3, 2021
@rustbot rustbot added A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. labels Jul 3, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jul 12, 2021
…riplett

stdio_locked: add tracking issue

Add the tracking issue number rust-lang#86845 to the stability attributes for the implementation in rust-lang#86799.

r? `@joshtriplett`
`@rustbot` label +A-io +C-cleanup +T-libs-api
@goffrie
Copy link
Contributor

goffrie commented Sep 30, 2021

Instead of adding fn into_locked, would it be a problem to instead change the signature of fn lock to return StdinLock<'static> directly? The latter is a subtype of StdinLock<'a> for any 'a, so I'd expect it to be backward compatible.

@tlyu
Copy link
Contributor Author

tlyu commented Oct 3, 2021

Instead of adding fn into_locked, would it be a problem to instead change the signature of fn lock to return StdinLock<'static> directly? The latter is a subtype of StdinLock<'a> for any 'a, so I'd expect it to be backward compatible.

I think when I brought up this possibility a while ago, there were objections that doing so would preclude certain future changes to the API.

@joshtriplett
Copy link
Member

This method has been around for a while, and it seems reasonable. I think it'd be appropriate to consider stabilizing it.

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Oct 25, 2021

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 25, 2021
@dtolnay
Copy link
Member

dtolnay commented Oct 26, 2021

change the signature of fn lock to return StdinLock<'static> directly

I think when I brought up this possibility a while ago, there were objections that doing so would preclude certain future changes to the API.

Would someone be able to dig up a link to this discussion? It seems like a better solution than introducing many new functions as long as the team is now willing to commit to the required API constraints.

@rfcbot concern can lock just do this

@dtolnay
Copy link
Member

dtolnay commented Oct 26, 2021

Would we still want these new APIs if let stdin = io::stdin().lock(); were supported by the borrow checker? I recall some long discussions about supporting this type of code. @joshtriplett does this ring a bell, or is it completely out of the question?

@dtolnay
Copy link
Member

dtolnay commented Oct 27, 2021

I found the "just make let stdin = io::stdin().lock() work" tracking issue: #15023. There is an accepted RFC. It looks not necessarily active but not out of favor either; someone just needs to do the work.

@joshtriplett
Copy link
Member

As far as I can tell, #15023 mentions that such a change would to some extent be a breaking change.

@tlyu
Copy link
Contributor Author

tlyu commented Nov 1, 2021

change the signature of fn lock to return StdinLock<'static> directly

I think when I brought up this possibility a while ago, there were objections that doing so would preclude certain future changes to the API.

Would someone be able to dig up a link to this discussion? It seems like a better solution than introducing many new functions as long as the team is now willing to commit to the required API constraints.

@rfcbot concern can lock just do this

I can try to locate the discussion; it might have been a Zulip thread or two. It was also a while ago (May or June 2021?), so it might take substantial digging.

@tlyu
Copy link
Contributor Author

tlyu commented Nov 1, 2021

@dtolnay I think I found it, though it kind of meanders. I personally don't agree that it's worth it to maintain future implementation flexibility (to a hypothetical design similar to one that was already rejected) at the cost of making things more difficult for beginners.

https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/What.20does.20StdinLock.20borrow.20from.20Stdin.3F

@iwikal
Copy link

iwikal commented Feb 13, 2022

Even if stdin().lock() was supported by the borrow checker, this API would be useful in situations where, even if the lifetime of the temporary was extended to the end of the scope, it would not suffice. Right now I'm in a situation where I can't use StdinLock because I need to move it into a static closure.

@joshtriplett
Copy link
Member

Cancelling in favor of #93965

@rfcbot cancel

@rfcbot
Copy link

rfcbot commented Feb 23, 2022

@joshtriplett proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Feb 23, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Mar 4, 2022
Make regular stdio lock() return 'static handles

This also deletes the unstable API surface area previously added to expose this
functionality on new methods rather than built into the current set.

Closes rust-lang#86845 (tracking issue for unstable API needed without this)

r? ````@dtolnay```` to kick off T-libs-api FCP
@bors bors closed this as completed in cdfb39e Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants