Using heap allocated FnOnce from a struct correctly

 use std::sync::Arc;
 
 struct Test {
  f: Arc<dyn FnOnce(String) -> String>,
 }
 
 fn main() {
    let s = "world";
    let f = Arc::new(move |x: String| x + s);
    let t = Test {f};
    let f2 = t.f.clone();
    println!("{}!", f2("hello ".to_owned()));
 }

(Playground)

Errors:

   Compiling playground v0.0.1 (/playground)
error[E0161]: cannot move a value of type dyn FnOnce(String) -> String: the size of dyn FnOnce(String) -> String cannot be statically determined
  --> src/main.rs:12:21
   |
12 |     println!("{}!", f2("hello ".to_owned()));
   |                     ^^

error[E0507]: cannot move out of an `Arc`
  --> src/main.rs:12:21
   |
12 |     println!("{}!", f2("hello ".to_owned()));
   |                     ^^ move occurs because value has type `dyn FnOnce(String) -> String`, which does not implement the `Copy` trait

error: aborting due to 2 previous errors

Some errors have detailed explanations: E0161, E0507.
For more information about an error, try `rustc --explain E0161`.
error: could not compile `playground`

To learn more, run the command again with --verbose.

How do I use this correctly? I want to do this to solve a complicated setup with several callbacks in wasm, which calls a callback in the end like this.

Is there a reason you are using FnOnce here? Putting FnOnce inside of Arc seems strange to me, since FnOnce can only be called once which makes sharing it not very useful.

FWIW, changing it from FnOnce to be Fn does work.

3 Likes

It's impossible to call an FnOnce if it's in an Arc. Consider putting it in a Box instead or using the Fn trait.

3 Likes

The real problem I'm trying to solve requires a FnOnce, apparently, since it moves a variable out of it's context. In this application the function is supposed to be called only once. The code in question is like this:

                  let final_callback = Arc::new(Box::new(move |ptr: Arc<Mutex<Defs>>| {
                    let mut env = mutex_env_c.lock().unwrap();
                    let defs = ptr.clone().into_inner().unwrap();
                    match file::check_all(p2, Rc::new(defs), store_c1) {
                      Ok(ds) => {
                        env.defs.flat_merge_mut(ds);
                      }
                      Err(e) => {
                        log!("Type checking failed. {:?}", e);
                      }
                    }
                  }) as Box<dyn FnOnce(Arc<Mutex<Defs>>)>);

store_c1 and p2 are moved. Can I make this into a dyn Fn(_) somehow? I get an error when I try to change this.

You can sometimes use an Option with its take method. It will give you ownership of the value, leaving a None in its place.

2 Likes

For answering this question, both the concrete error message as well as the surrounding code that the captured variables mutex_env_c, p2, and store_c1 come from are relevant.

(Especially the type of those variables would be good to know; I’m suspecting that either one or both of p2 and store_c1 are the reason why your closure doesn’t support Fn, but only FnOnce.)

Bigger context: types store: Rc<Store>, mutex_env: Arc<Mutex<ReplEnv>>, p2: Rc<Package>

                let store_c = store.clone();
                let mutex_env_c = mutex_env.clone();
                let callback = Box::new(move |ipld| {
                  let p = Rc::new(Package::from_ipld(&ipld).unwrap());
                  let p2 = p.clone();
                  let store_c1 = store_c.clone();
                  // This is called when all other callbacks has completed
                  let final_callback = Arc::new(Box::new(move |ptr: Arc<Mutex<Defs>>| {
                    let mut env = mutex_env_c.lock().unwrap();
                    let defs = ptr.clone().into_inner().unwrap();

                    match file::check_all(p2.clone(), Rc::new(defs), store_c1) {
                      Ok(ds) => {
                        env.defs.flat_merge_mut(ds);
                      }
                      Err(e) => {
                        log!("Type checking failed. {:?}", e);
                      }
                    }
                  }) as Box<dyn FnOnce(_)>);
                  let mon = CallbackMonitor::<Defs>::new(final_callback);
                  store::load_package_defs(store_c.clone(), p, Some(mon));
                });

And the CallbackMonitor (slightly modified):

#[derive(Clone)]
pub struct CallbackMonitor<T> {
  pub result: Arc<Mutex<T>>,
  callback_completed: Arc<Mutex<HashMap<String, bool>>>,
  final_callback: Arc<Box<dyn FnOnce(Arc<Mutex<T>>)>>,
  executed: bool,
}

I guess I could remove the Clone requirement somehow

Thy type of p2 is Rc, so your resulting Arc<dyn Fn…> can never be Arc<dyn Fn… + Send>, i. e. be threadsafe, anyway. Perhaps systematic use of only Rc instead of Arc makes more sense for single-threaded use. OTOH, your code seem to use mutexes, so maybe it is multi-threaded and the use of Rc will eventually turn out to be problematic...

In any case, back to the Fn vs FnOnce topic: I wouldn't be surprised if writing store_c1.clone() instead of store_c1 is all that's needed now to make it a dyn Fn instead of just FnOnce. Of course too much cloning reference counted pointers not “the best” approach if that closure really only is intended to be called once. But it isn't that much overhead either, and perhaps you're mostly after getting things to work in the first place right now.

Right, that's another approach. If you're only planning on calling final_callback once, then you won't need that derive(Clone). Then the Arc can go away, too. Removing Arcs (and Mutexes) by having single ownership when shared ownership really isn't needed can also make your code more idiomatic.

2 Likes

I found a solution! I removed derive Clone and wrapped the Monitor in a Arc<Mutex<>>.

/// Monitors the execution of several callbacks.
pub struct CallbackMonitor<T> {
  pub result: Option<T>,
  callback_completed: HashMap<String, bool>,
  final_callback: Option<Box<dyn FnOnce(T)>>,
  executed: bool,
}

impl<T> CallbackMonitor<T> {
  pub fn new<U: Default>(final_callback: Option<Box<dyn FnOnce(U)>>) -> CallbackMonitor<U> {
    let result = Default::default();
    let callback_completed: HashMap<String, bool> = Default::default();
    CallbackMonitor { result, callback_completed, final_callback, executed: false }
  }

  /// Tell the monitor to wait until notified by this callback id
  pub fn register_callback(&mut self, id: String) { self.callback_completed.insert(id, false); }

  /// Let the monitor know that a callback has completed.
  pub fn notify(&mut self, id: String) {
    let mons = &mut self.callback_completed;
    debug!("Notified by {}", &id);
    mons.insert(id, true);
    if mons.values().all(|&b| b) {
      debug!("final_callback");
      if let Some(fc) = self.final_callback.take() {
        if let Some(result) = self.result.take() {
          fc(result);
          self.executed = true;
        }
      }
    }
  }

  pub fn update_result(&mut self, f: Box<dyn FnOnce(&mut T)>) {
    if let Some(res) = &mut self.result {
      f(res);
    }
  }
}

This compiles

1 Like

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.