Moving a closure callback outside of the function

wuavcv · December 1, 2024, 11:53am

I was mentally compliing this issue before it got solved so I'm adding it for reference.

I had a long callback, inside a closure, that was distracting me from the main logic. So I wanted to move it outside the closure/calling-function entirely.

pub fn data_import_stage_1(model: &Model, args: &Args) {
    let reader = FileReader::new().expect("Failed to create FileReader");
    // snip....

    let mut model_copy = model.clone();
    let mut args_copy = args.clone();
    let reader_copy = reader.clone();

    ==============================
    let mut PROCESS_IMPORT_INSIDE = move || {
        // snip....
        let json = &reader_copy
        // snip....
        args_copy.entries.insert(/* ... */);
        controller::stg_data_import(2, &mut model_copy, &mut args_copy);
    };
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ VERY LONG CALLBACK I WANT TO MOVE
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 

    let reader_event = move |event: ProgressEvent| {
        // snip....
        match event.type_().as_str() {
            // snip....
            "loadend" => {
                // snip....
                PROCESS_IMPORT_INSIDE();
            }
        }
    };

    let add_event_listeners = || {
        let closure = Closure::wrap(Box::new(reader_event) as Box<dyn FnMut(ProgressEvent)>);
        let closure_ref = closure.as_ref().unchecked_ref();
        // snip....
        reader.set_onloadend(Some(closure_ref));
        closure.forget();
    };

    if let Some(files) = files {
        if let Some(file) = files.get(0) {
            // snip....
            add_event_listeners();
            reader.read_as_text(&file).expect("Ok: `()`");
        }
    }
}

This is how it started:

fn PROCESS_IMPORT_OUTSIDE(mut model_copy: Model, mut args_copy: Args, reader_copy: FileReader) {
    // snip....
    let json = &reader_copy
    // snip....
    args_copy.entries.insert(/* ... */);
    controller::stg_data_import(2, &mut model_copy, &mut args_copy);
}

pub fn data_import_stage_1(model: &Model, args: &Args) {
    let reader = FileReader::new().expect("Failed to create FileReader");

    let mut model_copy = model.clone();
    let mut args_copy = args.clone();
    let reader_copy = reader.clone();

    let reader_event = move |event: ProgressEvent| {
        // snip....
        match event.type_().as_str() {
            // snip....
            "loadend" => {
                // snip....
                PROCESS_IMPORT_OUTSIDE(model_copy, args_copy, reader_copy);
            }
        }
    };

    let add_event_listeners = || {
        let closure = Closure::wrap(Box::new(reader_event) as Box<dyn FnMut(ProgressEvent)>);
        let closure_ref = closure.as_ref().unchecked_ref();
        // snip....
        reader.set_onloadend(Some(closure_ref));
        closure.forget();
    };

    if let Some(files) = files {
        if let Some(file) = files.get(0) {
            // snip....
            add_event_listeners();
            reader.read_as_text(&file).expect("Ok: `()`");
        }
    }
}

But then I got this, which sent me down a rabbit hole of setting the Box to dyn FnOnce

  --> src/view/settings/create.rs:68:24
   |
68 | ...t reader_event = move |event: ProgressEvent| {
   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^ this closure implements `FnOnce`, not `FnMut`
...
81 | ...          process_import(model_copy, args_copy, reader_copy);
   |                             ---------- closure is `FnOnce` because it moves the variable `model_copy` out of its environment
...
88 | ...  let closure = Closure::wrap(Box::new(reader_event) as Box<dyn FnMut(Progress...
   |                                  ---------------------- the requirement to implement `FnMut` derives from here
   |
   = note: required for the cast from `Box<{closure@src/view/settings/create.rs:68:24: 68:51}>` to `Box<dyn FnMut(ProgressEvent)>`

Which made things worse for me when trying to get FnOnce to work...

error[E0277]: the trait bound `dyn FnOnce(ProgressEvent): WasmClosure` is not satisfied
   --> src/view/settings/create.rs:88:37
    |
88  | ... = Closure::wrap(Box::new(reader_event) as Box<dyn FnOnce(ProgressEvent)>);
    |       ------------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `WasmClosure` is not implemented for `dyn FnOnce(ProgressEvent)`
    |       |
    |       required by a bound introduced by this call
    |
    = help: the following other types implement trait `WasmClosure`:
              (dyn Fn() -> R + 'static)
              (dyn Fn(A) -> R + 'static)
              (dyn Fn(A, B) -> R + 'static)
              (dyn Fn(A, B, C) -> R + 'static)
              (dyn Fn(A, B, C, D) -> R + 'static)
              (dyn Fn(A, B, C, D, E) -> R + 'static)
              (dyn Fn(A, B, C, D, E, F) -> R + 'static)
              (dyn Fn(A, B, C, D, E, F, G) -> R + 'static)
            and 12 others
note: required by a bound in `Closure::<T>::wrap`
   --> /rust/cargo/registry/src/index.crates.io-6f17d22bba15001f/wasm-bindgen-0.2.95/src/closure.rs:254:17
    |
254 |     T: ?Sized + WasmClosure,
    |                 ^^^^^^^^^^^ required by this bound in `Closure::<T>::wrap`
...
281 |     pub fn wrap(mut data: Box<T>) -> Closure<T> {
    |            ---- required by a bound in this associated function

error[E0599]: the method `forget` exists for struct `Closure<dyn FnOnce(ProgressEvent)>`, but its trait bounds were not satisfied
  --> src/view/settings/create.rs:92:17
   |
92 |         closure.forget();
   |                 ^^^^^^ method cannot be called on `Closure<dyn FnOnce(ProgressEvent)>` due to unsatisfied trait bounds
   |
   = note: the following trait bounds were not satisfied:
           `dyn FnOnce(ProgressEvent): WasmClosure`

After giving up on FnOnce. I thought it must be the placement of my parameter copies. So I moved them closer to the function that uses them.

fn PROCESS_IMPORT_OUTSIDE(mut model_copy: Model, mut args_copy: Args, reader_copy: FileReader) {
    // snip....
    let json = reader_copy
    // snip....
    args_copy.entries.insert(/* ... */);
    controller::stg_data_import(2, &mut model_copy, &mut args_copy);
}

pub fn data_import_stage_1(model: &Model, args: &Args) {
    let reader = FileReader::new().expect("Failed to create FileReader");

    let reader_event = move |event: ProgressEvent| {

        ===== ARGS ARE NOW INSIDE READER_EVENT =====
        let model_copy = model.clone();
        let args_copy = args.clone();
        let reader_copy = reader.clone();

        // snip....
        match event.type_().as_str() {
            // snip....
            "loadend" => {
                // snip....
                PROCESS_IMPORT_OUTSIDE(model_copy, args_copy, reader_copy);
            }
        }
    };

    let add_event_listeners = || {
        let closure = Closure::wrap(Box::new(reader_event) as Box<dyn FnMut(ProgressEvent)>);
        let closure_ref = closure.as_ref().unchecked_ref();
        // snip....
        reader.set_onloadend(Some(closure_ref));
        closure.forget();
    };

    if let Some(files) = files {
        if let Some(file) = files.get(0) {
            // snip....
            add_event_listeners();
            reader.read_as_text(&file).expect("Ok: `()`");
        }
    }
}

But then I ran into this, which tripped me up for about TWO DAYS with me trying everyting under the sun to get the model/args parameters to live as long as the reader parameter.

Things I tried:

Adding lifetimes.
Cloning inline rather than using binding variables;
Moving the clones back outside the block.
Changing the signature of the parent function to use clones instead of borrows.
Reading an excellent blog on closures by @RustyYato Closures: Magic Functions
and so on...
and so on...

error[E0382]: borrow of moved value: `reader`
  --> src/view/settings/create.rs:89:31
   |
58 |     let reader = FileReader::new().expect("Failed to create FileReader");
   |         ------ move occurs because `reader` has type `FileReader`, which does not implement the `Copy` trait
...
65 |     let reader_event = move |event: ProgressEvent| {
   |                        --------------------------- value moved into closure here
...
69 |         let reader_copy = reader.clone();
   |                           ------ variable moved due to use in closure
...
89 |     let add_event_listeners = || {
   |                               ^^ value borrowed here after move
...
92 |         reader.set_onprogress(Some(closure_ref));
   |         ------ borrow occurs due to use in closure

error: lifetime may not live long enough
  --> src/view/settings/create.rs:90:37
   |
57 | ...l: &Model, args: &Args) {
   |       - let's call the lifetime of this reference `'1`
...
90 | ...wrap(Box::new(reader_event) as Box<dyn FnMut(ProgressEvent)>);
   |         ^^^^^^^^^^^^^^^^^^^^^^ cast requires that `'1` must outlive `'static`

error: lifetime may not live long enough
  --> src/view/settings/create.rs:90:37
   |
57 | ...: &Model, args: &Args) {
   |                    - let's call the lifetime of this reference `'2`
...
90 | ...rap(Box::new(reader_event) as Box<dyn FnMut(ProgressEvent)>);
   |        ^^^^^^^^^^^^^^^^^^^^^^ cast requires that `'2` must outlive `'static`

wuavcv · December 1, 2024, 11:54am

After trying to get help on IRC (not a great experience), I went back to bruteforcing combinations of borrowing, cloning, and variable location. I figured that if I failed then I would just leave the long callback inside the function.

But then after reverting changes for the umpteenth time, and trying to change as little as possible at each step. I found a permutation that compiles.

fn PROCESS_IMPORT_OUTSIDE(model: &mut Model, args: &mut Args, reader: &FileReader) {
    // snip....
    let json = &reader_copy
    // snip....
    args_copy.entries.insert(/* ... */);
    controller::stg_data_import(2, model, args);
}

pub fn data_import_stage_1(model: &Model, args: &Args) {
    let reader = FileReader::new().expect("Failed to create FileReader");

    let mut model_copy = model.clone();
    let mut args_copy = args.clone();
    let reader_copy = reader.clone();

    let reader_event = move |event: ProgressEvent| {
        // snip....
        match event.type_().as_str() {
            // snip....
            "loadend" => {
                // snip....
                PROCESS_IMPORT_OUTSIDE(&mut model_copy, &mut args_copy, &reader_copy);
            }
        }
    };

    let add_event_listeners = || {
        let closure = Closure::wrap(Box::new(reader_event) as Box<dyn FnMut(ProgressEvent)>);
        let closure_ref = closure.as_ref().unchecked_ref();
        // snip....
        reader.set_onloadend(Some(closure_ref));
        closure.forget();
    };

    if let Some(files) = files {
        if let Some(file) = files.get(0) {
            // snip....
            add_event_listeners();
            reader.read_as_text(&file).expect("Ok: `()`");
        }
    }
}

In the end, the solution was not so much what the compilier was suggesting (changing the closure type, changing the lifetimes of the parameters, etc...) -- it was to make sure that I am using/passing references, and that in the case of the model/args, that they are also mutable.

parasyte · December 1, 2024, 4:22pm

FWIW, you should not need to clone the captures. It is often enough to move borrows into the reader_event closure.

    let reader_event = {
        // These borrows are moved into the closure, shadowing the original variables.
        let model = &mut model;
        let args = &mut args;
        let reader = &reader;

        move |event: ProgressEvent| {
            // snip....
            match event.type_().as_str() {
                // snip....
                "loadend" => {
                    // snip....
                    PROCESS_IMPORT_OUTSIDE(&mut model, &mut args, &reader);
                }
            }
        }
    };

This is most often problematic when the original variables also need to be used later. If the code is taken at face value, reader might need to be cloned if its read_as_text() method takes &mut self or self. But model and args are not used later, cloning may not be required.

Moving ownership might be preferred if the closure needs to outlive the stack frame in which it's created.

wuavcv · December 1, 2024, 7:20pm

Yeah. I do need the variables later. I get the lifetime issues again when I replace the clones with mutable borrows. I'll give it another shot later.

But I do like the reader_event = { /* vars */ |event|{/*...*/} } syntax you used. It does make the scope more explicit.

Topic		Replies	Views
How can I move a closure into a closure within a closure? help	5	1406	January 12, 2023
Copying closure within closure help	6	66	December 21, 2024
Clone when moving into a closure help	6	3349	January 11, 2023
Sending to async method seems to lead to an impossible move	9	667	April 13, 2023
Why closure with move is not FnOnce	47	1010	September 5, 2024

Moving a closure callback outside of the function

Related topics