Use futures::lock::Mutex in Stdin

[kpp]

Right now you use std::sync::Mutex in Stdin which can block an entire reactor.

async-std/src/io/stdin.rs

Line 32 in c6e4c65

Stdin(Mutex::new(State::Idle(Some(Inner {

The solution for this flaw is to use futures::lock::Mutex.

Solution №1

pub struct Stdin(futures::lock::Mutex<Inner>); is not enough because futures::lock::Mutex::lock returns futures::lock::MutexLockFuture<'a> which depends on &self, and blocking::spawn expects Future to be 'static,. Which is sad =(

In the first commit I implemented it as pub struct Stdin(Arc<futures::lock::Mutex<io::Stdin>>); which allows to .clone() the data and makes blocking::spawn happy. However the lock is acquired inside blocking::spawn which is not cool, because we need to spawn a thread (potentially) to try to lock the mutex. And I could not reuse buffers from stdin.rs/Inner (line, buf), so I had to allocate the buffers every time you make a call to Stdin::read_line or Stdin::poll_read.

Solution №2

pub struct Stdin(Mutex<Arc<StdMutex<Inner>>>); is what we want. It uses 2 mutexes:

    pub async fn read_line(&self, buf: &mut String) -> io::Result<usize> {
        let future_lock = self.0.lock().await; // future mutex

        let mutex = future_lock.clone(); // 
        // Start the operation asynchronously.
        let handle = blocking::spawn(async move {
            let mut guard = mutex.lock().unwrap(); // std mutex
            let inner: &mut Inner = &mut guard;

The first mutex is a futures-aware mutex. It will lock the data without blocking an entire reactor.
Arc is a solution to make blocking::spawn happy.
StdMutex is a solution to sync the Inner data, because we need to call blocking::spawn and after that get the data to the parent "thread" to write it into buf: &mut String (in case of read_line).

Does it look like a working solution?

BTW I reduced the state machine and the code is much easier to read.

[yoshuawuyts]

This introduces a dependency on futures-util which we just removed. If this is a problem we should probably file an issue for it and design a solution how to solve it.

Either way, we can't land the current direction of this patch, so rather than keeping it open indefinitely I'm going to go ahead and close this. Thanks!

[kpp]

rather than keeping it open indefinitely I'm going to go ahead and close this

Rather than closing this PR one could switch from futures::lock::Mutex to async-std::sync::Mutex.