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

Back io::stdin with a global singleton BufferedReader #19416

Merged
merged 1 commit into from
Dec 5, 2014

Conversation

sfackler
Copy link
Member

io::stdin returns a new BufferedReader each time it's called, which
results in some very confusing behavior with disappearing output. It now
returns a StdinReader, which wraps a global singleton
Arc<Mutex<BufferedReader<StdReader>>. Reader is implemented directly
on StdinReader. However, Buffer is not, as the fill_buf method is
fundamentaly un-thread safe. A lock method is defined on StdinReader
which returns a smart pointer wrapping the underlying BufferedReader
while guaranteeing mutual exclusion.

Code that treats the return value of io::stdin as implementing Buffer
will break. Add a call to lock:

io::stdin().read_line();
// =>
io::stdin().lock().read_line();

Closes #14434

[breaking-change]

@rust-highfive
Copy link
Collaborator

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@sfackler
Copy link
Member Author

r? @aturon

@sfackler sfackler force-pushed the global-stdin branch 4 times, most recently from fa47a0e to 69a9f66 Compare November 30, 2014 18:44
@aturon
Copy link
Member

aturon commented Dec 4, 2014

@sfackler Sorry again for the delay in reviewing this -- it's been very hectic.

@alexcrichton and I talked at length about this PR today, in the context of our in progress std::io reform, and determined that you've basically hit the nail on the head with this. The only request we have is to add inherent methods to StdinReader corresponding to the default methods of Buffer (so not fill_buf or consume), which are implemented by calling the same method on the lock guard. That way the ergonomics are largely the same as today, the breakage is minimized, but we still ensure a clear, thread-safe semantics for the high-level operations.

@sfackler
Copy link
Member Author

sfackler commented Dec 4, 2014

Cool, I was considering doing that. read_line, read_until and read_char will be easy. chars and lines don't have a trivial delegation strategy since the lifetimes of the iterators they return won't work out.

I see three possibilities for those.

  • We can leave off those methods and force people to call lock() to get at them.
  • We can implement our own versions that hold the lock for the lifetime of the iterator object.
  • We can implement our own versions that only lock for each individual next call.

I think the first option is probably the best for now, since if we have chars and lines, I imagine it'll be a bit confusing to readers as to which of the options above was used and I'm not sure what the right answer is here, though I'd probably lean towards bullet 2.

@aturon
Copy link
Member

aturon commented Dec 4, 2014

I would actually lean more toward bullet 3 (since that's rougly the behavior you'd get by repeatedly calling read_line), but fine to leave these off for now.

@sfackler sfackler force-pushed the global-stdin branch 2 times, most recently from 42000e2 to 22a0d72 Compare December 4, 2014 07:08
@sfackler
Copy link
Member Author

sfackler commented Dec 4, 2014

@aturon updated

@sfackler
Copy link
Member Author

sfackler commented Dec 4, 2014

I also adjusted lock to take &mut self, which should help prevent people from deadlocking themselves.

io::stdin returns a new `BufferedReader` each time it's called, which
results in some very confusing behavior with disappearing output. It now
returns a `StdinReader`, which wraps a global singleton
`Arc<Mutex<BufferedReader<StdReader>>`. `Reader` is implemented directly
on `StdinReader`. However, `Buffer` is not, as the `fill_buf` method is
fundamentaly un-thread safe. A `lock` method is defined on `StdinReader`
which returns a smart pointer wrapping the underlying `BufferedReader`
while guaranteeing mutual exclusion.

Code that treats the return value of io::stdin as implementing `Buffer`
will break. Add a call to `lock`:

```rust
io::stdin().lines()
// =>
io::stdin().lock().lines()
```

Closes rust-lang#14434

[breaking-change]
@alexcrichton alexcrichton merged commit e7c1f57 into rust-lang:master Dec 5, 2014
@sfackler sfackler deleted the global-stdin branch December 6, 2014 03:23
muggenhor added a commit to muggenhor/rudoku that referenced this pull request Jan 4, 2015
Buffered reading from stdin now needs to lock stdin. This is to solve
rust-lang/rust#14434 which was a race condition on buffered reads from
stdin.

This change is to conform to the new API submitted as part of
rust-lang/rust#19416.
muggenhor added a commit to muggenhor/rudoku that referenced this pull request Jan 4, 2015
Buffered reading from stdin now needs to lock stdin. This is to solve
rust-lang/rust#14434 which was a race condition on buffered reads from
stdin.

This change is to conform to the new API submitted in
rust-lang/rust@e7c1f57 as part of
rust-lang/rust#19416.
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.

std::io::stdin() is causing significant trouble, losing data to its buffer
4 participants