WidgetDriver: Use matrix_sdk_common::executor::spawn instead of tokio::spawn to make it wasm compatible.

[toger5]

Using the executer::spawn will select tokio::spawn or wasm_bindgen_futures::spawn_local based on what platform we are on.

They behave differently in that tokio actually starts the async block inside the spawn but the wasm spawn wrapper will only start the part that is not inside the async block and the join handle needs to be awaited for it to work.

• Public API changes documented in changelogs (optional)

Signed-off-by:

[toger5]

I think I need help on this since this is not the correct solution. When running on a non wasm platform the await will block.
So there seems to be an issue with

#[cfg(target_arch = "wasm32")]
pub fn spawn<F, T>(future: F) -> JoinHandle<T>
where
    F: Future<Output = T> + 'static,
{
    let (future, remote_handle) = future.remote_handle();
    let (abort_handle, abort_registration) = AbortHandle::new_pair();
    let future = Abortable::new(future, abort_registration);

    wasm_bindgen_futures::spawn_local(async {
        // Poll the future, and ignore the result (either it's `Ok(())`, or it's
        // `Err(Aborted)`).

        let _ = future.await;
    });
    let j = JoinHandle { remote_handle: remote_handle, abort_handle };
    j.
}

Where we need to await the JoinHandle before the future is actually started on WASM.

[jplatte]

Adding the await there is definitely the wrong way to go. I don't see ny reason why it would be required on the wasm side, the docs for wasm_bindgen_futures state quite clearly that it's scheduled to run in the background, so does not need to be awaited. And the abstraction in mtrix-sdk-common would be worse than useless if this was not the case.

[toger5]

I have done a simple experiment in which this is failing however.

matrix_sdk_common::executor::spawn({
                info!("WIDGET_DRIVER a v1");
                async {
                    info!("WIDGET_DRIVER b v1");
                }
            });
    
            matrix_sdk_common::executor::spawn(async {
                info!("WIDGET_DRIVER b v2");
            });
    
            let c = "test".to_string();
            matrix_sdk_common::executor::spawn({
                let c = c.clone();
                info!("WIDGET_DRIVER a v3, {}", c);
                async move {
                    info!("WIDGET_DRIVER b v3 {}", c);
                }
            });

which results in the following outpu:

INFO src/main.rs:32 WIDGET_DRIVER a v1
INFO src/main.rs:45 WIDGET_DRIVER a v3, test

adding await to the returned handle (from spawn) makes it work.

[jplatte]

I wonder whether the AbortHandle aborts on drop? Maybe copy-paste bits of the spawn function and see if you can make it work without awaiting.

[toger5]

Maybe copy-paste bits of the spawn function and see if you can make it work without awaiting.

I tried this already. only using

    wasm_bindgen_futures::spawn_local(async {
        // Poll the future, and ignore the result (either it's `Ok(())`, or it's
        // `Err(Aborted)`).

          future.await;
    });

without calling remote_handle works as expected.
Checking if dropping the remove handle cancels the future is worth a try.

[codecov]

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.89%. Comparing base (bdf5fad) to head (3f544f1).
Report is 81 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/matrix-sdk/src/widget/mod.rs	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4707      +/-   ##
==========================================
- Coverage   85.90%   85.89%   -0.01%     
==========================================
  Files         292      292              
  Lines       33850    33851       +1     
==========================================
- Hits        29078    29076       -2     
- Misses       4772     4775       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jplatte]

only using

    wasm_bindgen_futures::spawn_local(async {
        // Poll the future, and ignore the result (either it's `Ok(())`, or it's
        // `Err(Aborted)`).

          future.await;
    });

without calling remote_handle works as expected.

Sounds like the wasm implementation of matrix_sdk_common::executor::spawn is buggy then. It should definitely behave roughly the same on all targets.

[toger5]

Sounds like the wasm implementation of matrix_sdk_common::executor::spawn is buggy then. It should definitely behave roughly the same on all targets.

That also was my assumption.
To sum up:

• tokio spawn will return a remote handle and dropping that handle does not stop the spawned procedure.

• spawn local with a remote handle will kill the future once the handle gets dropped.

  The handle to a remote future returned by remote_handle. When you drop this, the remote future will be woken up to be dropped by the executor.

  From the RemoteHandle docs

  let (future, remote_handle) = future.remote_handle();
  let (abort_handle, abort_registration) = AbortHandle::new_pair();
  let future = Abortable::new(future, abort_registration);
  
  wasm_bindgen_futures::spawn_local(async {
     // Poll the future, and ignore the result (either it's `Ok(())`, or it's
     // `Err(Aborted)`).
  
     let _ = future.await;
  });
  let j = JoinHandle { remote_handle: remote_handle, abort_handle };
  

  So the join handle from the wasm version is different.

What we would need is a way to keep the JoinHandle in scope so dropping the returned value does not kill the future.

[toger5]

I think this is what the forget method is doing.

[poljar]

I think this is what the forget method is doing.

To be honest, I prefer JoinHandles that require an explicit all for detachment. That is, if we need to modify a JoinHandle I'd rather we change the non-wasm one to require an explicit detach() or forget() call and drop() cancels the future on both platforms.

[toger5]

@poljar I think that is also easier. Since forget takes ownership of self (and returns ()) I think there is no clear way to do this.

So updating executor spawn for the tokio version sounds good. (and then we need the changes from this PR.)
I think most other locations in the code that use executor::spawn make sure they do not drop the join handle also.

[jplatte]

and then we need the changes from this PR.

No, then .detach() needs to be called in a bunch of places.

[toger5]

true, this PR already would be better if it uses detach/forget.