EspWebSocketClient cannot be shared across threads

[fangpenlin]

Bug description

I am trying to build a websocket client that can be shared with different threads for sending data over. I thought the web socket client is thread safe, but I soon realized that despite EspWebSocketClient implemented Send trait here

esp-idf-svc/src/ws/client.rs

Line 646 in 0115297

unsafe impl Send for EspWebSocketClient<'_> {}

the underlying c struct esp_websocket_client stops me from really sharing it across threads. I assume the Send trait for EspWebSocketClient meant to make it safe to share across threads?

Here are the sample code for reproducing it:

https://github.com/LaunchPlatform/esp-idf-svc-ws-client-cross-thread-bug/blob/f52412d1f8ce5a2a06c619713d4ab20609ae6443/src/main.rs#L18-L36

And the errors:

error[E0277]: `*mut esp_websocket_client` cannot be shared between threads safely
   --> src/main.rs:29:16
    |
29  |           .spawn(move || {
    |  __________-----_^
    | |          |
    | |          required by a bound introduced by this call
30  | |         // ^ And here we have the error:
31  | |         // `*mut esp_websocket_client` cannot be shared between threads safely
32  | |             loop {
...   |
35  | |             }
36  | |         })
    | |_________^ `*mut esp_websocket_client` cannot be shared between threads safely
    |
    = help: within `EspWebSocketClient<'_>`, the trait `Sync` is not implemented for `*mut esp_websocket_client`, which is required by `{closure@src/main.rs:29:16: 29:23}: Send`
note: required because it appears within the type `EspWebSocketClient<'_>`
   --> C:\Users\_\.cargo\registry\src\index.crates.io-6f17d22bba15001f\esp-idf-svc-0.50.0\src\ws\client.rs:405:12
    |
405 | pub struct EspWebSocketClient<'a> {
    |            ^^^^^^^^^^^^^^^^^^
    = note: required for `RwLock<EspWebSocketClient<'_>>` to implement `Sync`
    = note: required for `Arc<RwLock<EspWebSocketClient<'_>>>` to implement `Send`
note: required because it's used within this closure
   --> src/main.rs:29:16
    |
29  |         .spawn(move || {
    |                ^^^^^^^
note: required by a bound in `Builder::spawn`
   --> C:\Users\_\.rustup\toolchains\esp\lib\rustlib\src\rust\library\std\src\thread\mod.rs:372:12
    |
369 |     pub fn spawn<F, T>(self, f: F) -> io::Result<JoinHandle<T>>
    |            ----- required by a bound in this associated function
...
372 |         F: Send + 'static,
    |            ^^^^ required by this bound in `Builder::spawn`

• Would you like to work on a fix? [y/n]

I tried to workaround it by making esp_websocket_client comply with Send as well like

unsafe impl Send esp_websocket_client {}

But it seems like Rust compiler doesn't allow me to do it in my own module, not sure if there's anything can be done in binding generation?

To Reproduce

Clone my repo and build

https://github.com/LaunchPlatform/esp-idf-svc-ws-client-cross-thread-bug

Expected behavior

It should allow EspWebSocketClient to be shared between threads

Environment

• Crate (esp-idf-svc) version: 0.50
• ESP-IDF branch or tag: 5.3.2
• Target device (MCU): esp32s3
• OS: Windows11

[ivmarkov]

I am trying to build a websocket client that can be shared with different threads for sending data over. I thought the web socket client is thread safe, but I soon realized that despite EspWebSocketClient implemented Send trait here

esp-idf-svc/src/ws/client.rs

Line 646 in 0115297

unsafe impl Send for EspWebSocketClient<'_> {}

the underlying c struct esp_websocket_client stops me from really sharing it across threads. I assume the Send trait for EspWebSocketClient meant to make it safe to share across threads?

If, by "sharing across threads" you mean Arc-ing the thing (as you did) and then using it simultaneously from two threads (by cloning the Arc and then moving the cloned copy to the second thread), then - no - Send does not enable that.

What Send does it to allow you to create an object in one thread, and then move it to another thread. After the move however, the (main) thread where the object was created will not have access to the object, because now it will be owned by the second thread. A special Send marker is necessary, because some objects do not allow to be moved across threads (as they need to be pinned to the thread where they were created, due to thread-local data, whatever - a rare, but valid case).

To prove this, remove the Arc::new around the client and then keep everything else intact. The code will compile, as you have a move || in front of the 2nd thread closure, so the second thread will own the client, after you spawn the 2nd thread. You can even check that by trying to use - after the thread:spawn call - the client in the main thread, which would generate a compiler error, because the object was moved.

Now, if you want to use the client from two threads simultaneously, then you are reaching to something else called Sync. To do this with the client (which is not Sync out of the box), you need to Arc it as you did, but before that you need to also Mutex it. The reason for that is because the client has a bunch of mutable methods (like the send method you are trying to call from the 2nd thread) and also internal state which is not Sync-compatible (see below). Due to Rust aliasing rules, it is not allowed for one thread to call a &mut self method on an object, while this object is shared with another thread. If that were possible, the object would've been Sync, and this method would've been taking just &self. A Mutex is an easy way to turn a non-Sync object to a Sync one.

error[E0277]: *mut esp_websocket_client cannot be shared between threads safely

I tried to workaround it by making esp_websocket_client comply with Send as well like

unsafe impl Send esp_websocket_client {}

But it seems like Rust compiler doesn't allow me to do it in my own module, not sure if there's anything can be done in binding generation?

This is too long to explain in details, but here the compiler is actually telling you that one reason EspWebsocketClient is not automatically Sync (Send and Sync are also auto-traits, you should read on that) is because it contains a raw pointer (*mut esp_websocket_client). It is just extra paranoid with raw pointers. We can always override this by unsafe impl Sync for EspWebsocketClient but we should only do this if we believe the underlying implementation of the C driver is inherently thread-safe by using its own lock (which I'm not sure is the case).

[ivmarkov]

Btw, Send and Sync are very difficult to get intuition for, as they are unique to Rust.
Also move semantics is very difficult to get intuition for because it is also unique to Rust (C++ has something of a move impl, but it is so lame I don't think it counts). You should strive hard to get an intuition for "move" as it is a key corner-stone of Rust semantics.

[ivmarkov]

Also just noticed the error message from the compiler mentioned RwLock. Did you wrap it with rwlock in the meantime, as the source code you pointed to does not have this.

[ivmarkov]

If you are trying to use rwlock, use a mutex instead, because rwlock does not upgrade Send objects to Sync. See this: https://www.reddit.com/r/rust/comments/11fyjl7/why_is_rwlock_not_a_sync_maker/

[fangpenlin]

Yeah, I tried to simplified the sample code for reproducing the issue, so I didn't copy my RwLock part into the sample code. I read their source code, it seems like they apply mutex around, while their doc didn't say it clearly, I guess it's thread safe. Like this one:

https://github.com/espressif/esp-protocols/blob/542547d38bcd656aab3e8fd6f35dcfa5500dfcda/components/esp_websocket_client/esp_websocket_client.c#L848-L850

Thank you for clarifying the difference between Sync and Send, looks like I misunderstood the difference. Now I know that I can probably get away with a mutex, that helps a lot. The only problem would be there's already mutex applied in their c code, so it's a bit locking twice. But still, there are data struct in Rust may need protection, so maybe it's needed anyway.

I guess based on what you said, changing the Send trait added to EspWebSocketClient might make it possible to share the object without a mutex wrapping around it. However, it means we need to be sure that the EspWebSocketClient object is 100% thread safe (the fact that callback function likely going to be invoked from a different thread already implies it I guess? at least from the C code perspective?). But regardless, maybe it's a bad idea to share the client object across different threads in the same time anyway, even we can make it thread-safe. I think I would restructure my code to use thread safe queue or other ways to pull data or actions from other threads if needed.

Close this issue now. Thanks again 🙌

[ivmarkov]

Yeah, I tried to simplified the sample code for reproducing the issue, so I didn't copy my RwLock part into the sample code. I read their source code, it seems like they apply mutex around, while their doc didn't say it clearly, I guess it's thread safe. Like this one:

https://github.com/espressif/esp-protocols/blob/542547d38bcd656aab3e8fd6f35dcfa5500dfcda/components/esp_websocket_client/esp_websocket_client.c#L848-L850

Do they? What happens if I call esp_websocket_client_send_* from two threads simultaneously?

[ivmarkov]

Ah, they seem to take a lock here.

So it is only start/stop then which are problematic.