False positive for await_holding_refcell_ref: RefMut for keeping the awaited future alive

[MattiasBuelens]

Lint name: await_holding_refcell_ref

I tried this code:

#[wasm_bindgen]
struct IntoUnderlyingSink {
    inner: Rc<RefCell<Inner>>,
}

#[wasm_bindgen]
impl IntoUnderlyingSink {
    pub fn write(&mut self, chunk: JsValue) -> Promise {
        let inner = self.inner.clone();
        future_to_promise(async move {
            let mut inner = inner.try_borrow_mut().unwrap_throw();
            inner.write(chunk).await.map(|_| JsValue::undefined())
        })
    }
}

impl Inner {
    async fn write(&mut self, chunk: JsValue) -> Result<(), JsValue> {
        // ...
    }
}

(For more context, see the complete code.)

This might seem a bit of a contrived example, but it's actually the only way I found to use async fn inside a Rust struct exported to JavaScript with wasm-bindgen. 🤷 (Better suggestions are welcome though!)

I expected to see this happen: No error since the RefMut is necessary to keep the inner.write(chunk) future alive.

Instead, this happened: Causes a warning.

error: this RefCell Ref is held across an 'await' point. Consider ensuring the Ref is dropped before calling await.
  --> src\writable\into_underlying_sink.rs:30:17
   |
30 |             let mut inner = inner.try_borrow_mut().unwrap_throw();
   |                 ^^^^^^^^^
   |
   = note: `#[deny(clippy::await_holding_refcell_ref)]` on by default
note: these are all the await points this ref is held through
  --> src\writable\into_underlying_sink.rs:30:13
   |
30 | /             let mut inner = inner.try_borrow_mut().unwrap_throw();
31 | |             inner.write(chunk).await.map(|_| JsValue::undefined())
32 | |         })
   | |_________^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_refcell_ref

Meta

• cargo clippy -V: clippy 0.0.212 (e1884a8e 2020-12-29)
• rustc -Vv:

  rustc 1.49.0 (e1884a8e3 2020-12-29)
  binary: rustc
  commit-hash: e1884a8e3c3e813aada8254edfa120e85bf5ffca
  commit-date: 2020-12-29
  host: x86_64-pc-windows-msvc
  release: 1.49.0
  

---

I noticed that this particular lint was already downgraded in #6354 for Rust 1.50 (currently in beta). I can confirm that the warning does not show up when using Rust beta or nightly with the default settings. Since it only shows up in Rust 1.49, I worked around it on my end by disabling this lint.

Still, I opened this issue since I think this might be an interesting false positive. Feel free to close if you disagree, no hard feelings. 🙂

[ileixe]

I encountered same error and wonder it can occur actual bad effect or not. I also was trying to save Future in RefCell.

Here is MRE which I could not make a panic.

use futures::stream::FuturesUnordered;
use futures::{Future, StreamExt};
use std::cell::RefCell;

async fn hello() {
    tokio::time::sleep(std::time::Duration::from_micros(1));
}

struct Me<T: Future> {
    queue: RefCell<FuturesUnordered<T>>,
}

impl<T: Future> Me<T> {
    fn push(&mut self, t: T) {
        self.queue.borrow_mut().push(t);
    }

    async fn next(&mut self) -> Option<T::Output> {
        self.queue.borrow_mut().next().await
    }
}
#[tokio::main]
async fn main() {
    let mut me = Me {
        queue: RefCell::new(FuturesUnordered::new()),
    };
    me.push(tokio::task::spawn(hello()));

    loop {
        tokio::select! {
            Some(_) = me.next() =>  {
                for _ in 0..100 {
                    me.push(tokio::task::spawn(hello()));
                }
            },
            else => break
        }
    }
}

Honestly, I could not understand how exactly "holding the RefMut at await" is harmful. Is it coming from the fact that when async fn yields, they save their captured variables (so as RefMut) and it may cause other async task touch the RefCell again?
If then how the code above did not panic?

[Storyyeller]

There isn't anything harmful about it, as long as you are doing it intentionally. Presumably the lint was added because people often do it by mistake when they really want something else.

I just came here from Google due to also getting a false alarm from this lint in a situation where holding RefMut across await is necessary but the structure of the code makes panics impossible (due to being guarded by an upstream &mut like in your example).

Note that in your example, you should be able to avoid the problem by changing the borrow_mut()s to get_mut()s, but this was not possible in my situation.