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

Locking for stdin #229

Closed
sophiajt opened this issue Sep 21, 2019 · 12 comments
Closed

Locking for stdin #229

sophiajt opened this issue Sep 21, 2019 · 12 comments
Labels
enhancement New feature or request

Comments

@sophiajt
Copy link

In sync std, you can lock stdin to ensure you maintain control of it. Something like:

let stdin = std::io::stdin();
let locked = stdin.lock();

Currently, there isn't a way to do the equivalent in async-std.

cc @yoshuawuyts

@k-nasa
Copy link
Member

k-nasa commented Sep 21, 2019

I want to try this implementation!
But I can't imagine what kind of implementation should be done (due to my lack of ability)
@yoshuawuyts Could you show me a little way?

@yoshuawuyts
Copy link
Contributor

@k-nasa yay, thanks for volunteering!

So there are two important things to be aware of here:

// definition in async-std, `State` holds a reference to std's Stdin
pub struct Stdin(Mutex<State>);

We want to have an async fn lock on Stdin that returns a new struct StdinLock which implements Read and BufRead.

The way I'd approach this by making our method acquire outer async lock, then call lock on the reference to std::io::Stdin and borrow std's StdinLock. Then from there we can forward calls, to it, and make sure we properly handle io::error::WouldBlock. Example code for this should be part of our Stdin code already.

Hope this provides a good starting point; let me know if you need any more pointers!

@yoshuawuyts yoshuawuyts added the enhancement New feature or request label Sep 22, 2019
@ghost
Copy link

ghost commented Sep 22, 2019

To add a little bit to that, since std::io::Stdin::lock() blocks, we would have to invoke that function inside task::blocking().

Moreover, the returned StdinLock<'_> will hold a reference to the Stdin instance, so we should probably make sure that it's 'static by putting it into lazy_static!:

lazy_static! {
    static ref STDIN: Stdin = std::io::stdin();
}

A locking function might then look like this:

use crate::task::blocking;

async fn lock() -> StdinLock<'static> {
    blocking::spawn(async move { STDIN.lock() }).await
}

@k-nasa
Copy link
Member

k-nasa commented Sep 23, 2019

@yoshuawuyts @stjepang Thx!! I try it.

@yoshuawuyts
Copy link
Contributor

ref #131

@k-nasa
Copy link
Member

k-nasa commented Oct 2, 2019

Apparently, StdinLock does not implement the Send trait. So it doesn't seem to work with blocking :: spawn.

--> src/io/stdin.rs:167:9
    |
167 |           blocking::spawn(async move {
    |           ^^^^^^^^^^^^^^^ `std::sync::MutexGuard<'_, std::io::BufReader<std::io::stdio::Maybe<std::io::stdio::StdinRaw>>>` cannot be sent between threads safely
    |
   ::: src/task/blocking.rs:101:1
    |
101 | / pub fn spawn<F, R>(future: F) -> JoinHandle<R>
102 | | where
103 | |     F: Future<Output = R> + Send + 'static,
104 | |     R: Send + 'static,
...   |
108 | |     JoinHandle(handle)
109 | | }
    | |_- required by `task::blocking::spawn`
    |
    = help: within `std::io::StdinLock<'_>`, the trait `std::marker::Send` is not implemented for `std::sync::MutexGuard<'_, std::io::BufReader<std::io::stdio::Maybe<std::io::stdio::StdinRaw>>>`
    = note: required because it appears within the type `std::io::StdinLock<'_>`
#[stable(feature = "rust1", since = "1.0.0")]
pub struct StdinLock<'a> {
    inner: MutexGuard<'a, BufReader<Maybe<StdinRaw>>>,
}

#[must_use = "if unused the Mutex will immediately unlock"]
#[stable(feature = "rust1", since = "1.0.0")]
pub struct MutexGuard<'a, T: ?Sized + 'a> {
    // funny underscores due to how Deref/DerefMut currently work (they
    // disregard field privacy).
    __lock: &'a Mutex<T>,
    __poison: poison::Guard,
}

What problems can be considered when locking is returned without blocking?
in this way.

    pub async fn lock(&self) -> std::io::StdinLock<'static> {
        STDIN.lock()
    }

@k-nasa
Copy link
Member

k-nasa commented Oct 4, 2019

ping @stjepang @yoshuawuyts
sorry

@yoshuawuyts
Copy link
Contributor

yoshuawuyts commented Oct 14, 2019

@k-nasa I've been thinking about this and don't really have any good suggestions; sorry. This almost feels like something that should be solved in stdlib to ensure the internal lock is always acquired even through multiple interfaces.

In lieu of that I think perhaps doing a blocking call directly inside the async block is fine for now. It's not ideal, but it provides the right interface.

We already have work stealing which means that even if a current thread blocks, other threads will pick up work. Which in turn means that the effects of blocking tasks will be mitigated as we continue to improve our executor.

edit though the guard here may make the current task non-send which is also not great. Having a spawn_local method might be nice also, but ooph yeah that's not great either.

It almost feels like perhaps we should have a global somewhere that we can instrument to take out the lock, and then write to it from other methods. And then once it's done close the lock again.

@ghost
Copy link

ghost commented Oct 14, 2019

I believe it should be just fine to use unsafe code to send StdinLock to another thread.

@kpp
Copy link
Contributor

kpp commented Oct 31, 2019

@k-nasa You may want to be aware of my solution: #122

@yoshuawuyts
Copy link
Contributor

@jonathandturner now that #334 is merged, the next release should work for your use case as part of the "unstable" feature set! 🎉

Apologies it took a few weeks to resolve, but the biggest hurdle has been overcome now!

@k-nasa
Copy link
Member

k-nasa commented Nov 6, 2019

Close because #334 has been merged

@k-nasa k-nasa closed this as completed Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants