Gd<T> cannot be passed as a parameter in async signals.

[ColinWttt]

It is unrelated to whether the signal is type-safe.

     #[signal]
     fn custom_signal_gd(value: Gd<RefCounted>);

This comes from the Send + Sync bounds here:

pub struct SignalFuture<R: ParamTuple + Sync + Send>(FallibleSignalFuture<R>);

Those bounds are unnecessary for signals that are emitted on the main thread. (Awaiting must anyway happen on the main thread).

Was the intention here to support also signals emitted on other threads, as a cross-thread communication mechanism? If yes, we should probably add this later -- might need more thought regarding thread safety, and probably some version of #18.

Originally posted by @Bromeon in #1043 (comment)

[TitanNano]

Those bounds are unnecessary for signals that are emitted on the main thread. (Awaiting must anyway happen on the main thread).

Was the intention here to support also signals emitted on other threads, as a cross-thread communication mechanism? If yes, we should probably add this later -- might need more thought regarding thread safety, and probably some version of https://github.com/godot-rust/gdext/issues/18.

Yes, it's basically impossible to tell where a signal will be emitted, since any signal can be emitted from any thread. We also use a RustCallable inside the future, and it has a Send + Sync bound. Making the future Send + Sync is the most reliable solution.

For Gd I was thinking about a follow-up PR where I would like to work out something like the ThreadCrosser in itest but with a runtime check if the value actually was moved between threads. I haven't worked on this yet, though.

Originally posted by @TitanNano in #1043 (comment)

[TitanNano]

I have started looking into a solution for this.

[coder137]

My recommendation is: https://github.com/smol-rs/async-task

From the discussion it seems like the plan is to check thread ids.
The async_task::spawn_local API panics if the runnable is invoked from a different thread from the one that spawns it.

Might also help reduce boiler plate for the gdext async runtime implementation.
If adding this as a third party isn't acceptable I hope it can be a good reference to remove the Send + Sync bound on signals.

[Bromeon]

Whatever we end up choosing, the API should follow the principle "easy-to-use for the common case, verbose for the rare cases".

I'd argue that 95%+ of users emit signals on the same thread that they await. Needing to go through extra hoops for passing Gd<T>, such as a a newtype that is Send/Sync, is a lot to ask for. I'd rather remove those bounds, and maybe @coder137's suggestion helps here.

We could have spawn and spawn_sync, similar to how Callable::from_local_fn + Callable::from_sync_fn work. The former would panic if it detects a thread crossing. As for naming, moving forward it's probably OK to assume that "local" is the default, while "thread-crossing"/"sync" is explicitly qualified. This is already done for ConnectBuilder, which has a sync() method but no local() one.

I also don't think we should make large-scale sweeping changes to threading across the library, or block this particular on #18 being solved. It seems this issue is isolated enough that we could solve the smaller problem, like we did for callables and typed signals?

[TitanNano]

Please note: The problem does not originate in the async runtime.

The bound is caused by the RustCallable in the FallibleSignalFuture which requires the Send + Sync bound. We can introduce a second LocalRustCallable that panics like Callable::from_local_fn, then we can use this callable and get rid of the bound. But This will require two implementations of all the futures. A FallibleSignalFuture and a SendFallibleSignalFuture as well as SignalFuture and SendSignalFuture. We also need to split all the Signal functions like to_future and to_send_future.

My counterproposal is that we check if the signal argument is being sent across threads and also !Send and if this happens we panic. This would not require any API changes at all.

Here is a POC how that could be implemented: TitanNano@798d222
(This is not 100% done for all possible argument types and needs some cleanup)

As you can see, nothing changes for the user, but now you can await signals with arguments that are not Send or Sync.