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

Fix an easy to trigger deadlock in std::io::stdio #23087

Merged
merged 1 commit into from
Mar 7, 2015

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Mar 5, 2015

Being a person who somehow has taken a liking to premature optimisation, my knee-jerk reaction to
locking in std handles was preamble resembling following snippet:

let stdout = stdout();
let lstdout = stdout.lock();
let stdin = stdin();
let lstdin = stdin.lock();

and then reading from the locked handle like this:

let mut letter = [0; 1];
lstdin.read(&mut letter).unwrap();

As it is now this code will deadlock because the read method attempts to lock stdout as well!

r? @alexcrichton


Either way, I find flushing stdout when stdin is used debatable. I believe people who write prompts should take care to flush stdout when necessary themselves.

Another idea: Would be cool if locks on std handles would be taken for a thread, rather than a handle, so given preamble (first code snippet)

stdin.lock()

or more generally

stdin.read(…)

worked fine. I.e. if more than a single lock are all taken inside the same thread, it would work, though not sure if our synchronisation primitives are expressive enough to make it possible.

@sfackler
Copy link
Member

sfackler commented Mar 5, 2015

I'd personally vote for just not flushing stdout at all there.

@alexcrichton
Copy link
Member

I agree that we should not be flushing, so could you delete the code entirely?

I.e. if more than a single lock are all taken inside the same thread, it would work, though not sure if our synchronisation primitives are expressive enough to make it possible

This definitely seems like a plausible idea. I'd have to mull it over a bit more but I imagine that it's an implementation detail we could add later on.

@alexcrichton
Copy link
Member

ah and r=me with the code deleted

Being a person who somehow has taken a liking to premature optimisation, my knee-jerk reaction to
locking in std handles was preamble resembling following snippet:

    let stdout = stdout();
    let lstdout = stdout.lock();
    let stdin = stdin();
    let lstdin = stdin.lock();

and then reading from the locked handle like this:

    let mut letter = [0; 1];
    lstdin.read(&mut letter).unwrap();

As it is now this code will deadlock because the `read` method attempts to lock stdout as well!
@nagisa
Copy link
Member Author

nagisa commented Mar 6, 2015

Updated.

@alexcrichton
Copy link
Member

@bors: r+ 3f94260

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 6, 2015
Being a person who somehow has taken a liking to premature optimisation, my knee-jerk reaction to
locking in std handles was preamble resembling following snippet:

    let stdout = stdout();
    let lstdout = stdout.lock();
    let stdin = stdin();
    let lstdin = stdin.lock();

and then reading from the locked handle like this:

    let mut letter = [0; 1];
    lstdin.read(&mut letter).unwrap();

As it is now this code will deadlock because the `read` method attempts to lock stdout as well!

r? @alexcrichton

---

Either way, I find flushing stdout when stdin is used debatable. I believe people who write prompts should take care to flush stdout when necessary themselves.

Another idea: Would be cool if locks on std handles would be taken for a thread, rather than a handle, so given preamble (first code snippet)

    stdin.lock()

or more generally

    stdin.read(…)

worked fine. I.e. if more than a single lock are all taken inside the same thread, it would work, though not sure if our synchronisation primitives are expressive enough to make it possible.
@bors bors merged commit 3f94260 into rust-lang:master Mar 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants