GetStatusChangeCall and GetStatusChangeReturn

[ibeckermayer]

Corresponds with gravitational/teleport#33056

[github-actions]

Coverage Report 🤖 ⚙️

Past:
Total lines: 23946
Covered lines: 15654 (65.37%)

New:
Total lines: 24088
Covered lines: 15654 (64.99%)

Diff: -0.39%

[this comment will be updated automatically]

[CBenoit]

This looks good to me, but now I’m curious about what is actually requiring the Sync bound on your side. Is it because you are sharing DeviceControlResponse (holding Box<dyn rpce::Encode>) in various threads, using something like Arc?

[ibeckermayer]

It's because we need to hold a DeviceControlResponse (holding Box<dyn rpce::Encode>) in our RdpdrBackend implementation, and RdpdrBackend is

RdpdrBackend: fmt::Debug + Send + Sync

That happens here in the Teleport code, with the comment explaining it at a high level.

[CBenoit]

Thank you, I see.

I note we have the same "problem" with CliprdrBackend.
I understand we need the Sync bound on the backend trait because it is required by StaticVirtualChannelProcessor, because in turn it is also required by ironrdp_connector::Sequence.
But it’s probably not very useful to require this at all: out of curiosity, I tried removing the bound and everything still works, at least in IronRDP repository, and to be fair I don’t think it’s sensible to share the connector or the client itself across multiple threads (like in wrapping it into a Mutex and driving it across many threads in parallel; that would just create more contention), so it would make sense if Sync was not required everywhere like it is today. EDIT: Actually, that’s not quite accurate; being only Send is way less restrictive that what I described. You can still share Mutex<T> with many threads and end up with a high contention (depending on the payload). That’s because as long as T is Send, Mutex<T> is both Send AND Sync.
However, MutexGuard<T> is not Sync if T is not Sync too, which means &MutexGuard<T> can’t be (cloned and) sent to other threads, and this also holds for &&mut T that you could obtain by dereferencing the MutexGuard<T> and borrowing the returned &mut T.
This is to be expected since both can be dereferenced or coerced into a &T and &T is Send iff T is Sync, so we’re all good: T can hold a Cell or RefCell because we’re guaranteed that there is at most one thread actually accessing it, that is: no concurrent access can happen and we are allowed to use such non-thread-safe primitives.

The reason I’m considering this carefully is (I’m sure you’ve guessed it) WASM. As you know, in WASM, when running in the browser, we can’t block the main thread at all, and this includes Mutexes.

Here is also a relevant thread on the Rust Users forum I came across not long ago:
https://users.rust-lang.org/t/wasm-winit-wgpu-weird-error-about-waiting-on-the-main-thread-despite-not-waiting-on-the-main-thread/96804/1

This means that, in the Web Client, if at some point for some reason we really need interior mutability in order to implement a backend, we can’t cheat our way using Mutex; Cell or RefCell would be the way to go. Incidentally, it’s also something that could be used in a multi-threaded application as long as the Cell itself is not shared, which is actually not useful as often as I initially thought, and when sharing a Cell among several tasks is actually required it’s possible even with tokio using a LocalSet.

Anyway, I’m not blocking the PR for this because it’s not directly related to it and this should be addressed in its own PR. It’s not necessarily urgent either. Pinging @pacmancoder for information as he’s currently implementing the cliprdr backend for the Web Client.

EDIT: Of course, nothing prevents us from having a Sync backend by using fully thread-safe primitives like holding an Arc<Mutex<T>> shared with something else in another thread
It’s just that we can’t make this assumption anymore once we manipulate it as a trait object using Box<dyn Backend>, but as said it’s unlikely to be a problem in practice (we’ll spawn new threads at instantiation time or directly from inside the backend implementation, where we know our type is Sync)
To begin with, we already kind of make this assumption by having almost all our trait methods taking &mut self, an exclusive reference which itself can’t be cloned and exist at two different places at the same time by definition (that would be UB).

[ibeckermayer]

Makes sense to me at a high level, this works for Teleport as well. I added this to this PR in 11b2e7e.

[ibeckermayer]

Oh, I realize you enabled auto-merge so I reverted that, I will put it up in a separate PR so you and @pacmancoder have a chance to confirm.

[ibeckermayer]

#209

[pacmancoder]

Yes, I completely agree that Sync should be removed

[CBenoit]

Looking good! CI is reporting some lints, but I’m approving and enabling auto-merge now so it’s automatically merged when addressed.