EspWebSocketClient::Drop still panics: esp_websocket_client_destroy unwrap

[fuk-yu]

PR #648 fixed the esp_websocket_client_close unwrap in Drop, but esp_websocket_client_destroy still calls .unwrap():

  impl Drop for EspWebSocketClient<'_> {
      fn drop(&mut self) {
          if let Err(e) = esp!(unsafe { esp_websocket_client_close(self.handle, self.timeout) }) {
              log::warn!("WebSocket close failed during drop: {e:?}");
          }
          esp!(unsafe { esp_websocket_client_destroy(self.handle) }).unwrap(); // can still panic
      }
  }

Why this is a problem

1. Drop must not panic. On ESP-IDF targets using panic = "abort", any panic in Drop aborts the entire application — there is no unwinding to catch it. A
   Drop impl should be infallible.
2. The new_raw construction order triggers this. The client struct is constructed before esp_websocket_client_start() is called. When start() fails (e.g.,
   server unreachable), the ? operator returns Err, dropping the already-constructed client. Drop then runs on a client whose transport was never established.
3. esp_websocket_client_destroy can return errors. While it typically returns ESP_OK, the ESP-IDF documentation does not guarantee this. Future ESP-IDF
   versions or edge cases (e.g., double-free, corrupted handle after a failed start) could cause it to fail.

Suggested fix

Apply the same pattern as close — or simpler, just ignore errors from both cleanup calls:

  impl Drop for EspWebSocketClient<'_> {
      fn drop(&mut self) {
          let _ = esp!(unsafe { esp_websocket_client_close(self.handle, self.timeout) });
          let _ = esp!(unsafe { esp_websocket_client_destroy(self.handle) });
      }
  }

Reproduction

1. Configure EspWebSocketClient with disable_auto_reconnect: true pointing at a non-existent server
2. Call EspWebSocketClient::new() — the TCP connection fails asynchronously
3. The client is dropped (either from new_raw error path or after detecting the failure)
4. Result: abort() was called — application crashes

Environment

• ESP32-P4, esp-idf-svc 0.52.1, ESP-IDF v5.4.3 / v5.5.4
• panic = "abort" (default for ESP-IDF targets)

[ivmarkov]

Is this error path happening for real, for you, or are you just pointing your LLM at random repos and letting it theorize?

If yes: I wonder whether we should instead fix the ctor fns of the websocket client Rust wrapper so that it does not instantiate an EspWebsocketClient struct until its components had been fully initialized as well. Related topic: esp-rs/esp-idf-hal#563 (comment)

Also, if esp_websocket_client_start can bail out with a real IO error (as opposed to programmatical "invalid args" / "invalid state") it might make sense to expose start and stop public fns on the Rust wrapper.

If not: I suggest you reproduce for real first.

[fuk-yu]

I've spent 2 hours trying to figure out why it panics when my server is not available during initial connection attempt. It reproduces. The Claude's fix works - I vendored esp-idf-svc, applied the patch and the crash is gone. I can't judge if that's the right way to fix but the problem indeed exists.

So yes, this error path is real for me. The code is in my only repo.

[ivmarkov]

I've spent 2 hours trying to figure out why it panics when my server is not available during initial connection attempt. It reproduces. The Claude's fix works - I vendored esp-idf-svc, applied the patch and the crash is gone. I can't judge if that's the right way to fix but the problem indeed exists.

So yes, this error path is real for me. The code is in my only repo.

Then can you please ask claude to fix the constructor fns instead?

.. and open a PR with the fix?

[fuk-yu]

I don't understand what exactly needs to be done, so I can't properly instruct the model / verify the results.

[ivmarkov]

I don't understand what exactly needs to be done, so I can't properly instruct the model / verify the results.

Why don't you let it read this conversation?

[fuk-yu]

Ok, I will - but again, even if it bakes another patch which would work, I won't be able to say if it's good. For my purpose even the current patch works - the firmware doesn't panic and to me it makes no actual difference if the patch is reasonable or not.

[ivmarkov]

Ok, I will - but again, even if it bakes another patch which would work, I won't be able to say if it's good. For my purpose even the current patch works - the firmware doesn't panic and to me it makes no actual difference if the patch is reasonable or not.

I get that, but I don't feel comfortable with merging quick patches when we don't have the full picture.

If it works for you this way, just use a forked repo.

[fuk-yu]

I don't feel comfortable with merging quick patches

That's totally fair.

If it works for you this way, just use a forked repo.

That's what I do, though the problem is real, so my thought was that it's worth pointing the dev attention to.

This is the patch it baked in response to your comment:

Subject: [PATCH] fix(ws/client): prevent Drop on partially-initialized EspWebSocketClient

The constructor used to create the `Self` struct before calling
`esp_websocket_register_events` and `esp_websocket_client_start`.
When either of those failed, the `?` operator would drop a
partially-initialized client, whose `Drop` impl called
`esp_websocket_client_close` / `esp_websocket_client_destroy`
on a handle that was never fully started — panicking on
`panic = "abort"` targets (all ESP-IDF).

Fix both the root cause and the symptom:

1. **Constructor**: defer `Self` construction until all fallible
   init steps succeed. On failure, manually destroy the raw handle
   and return the error — no `Self` is ever created, so `Drop`
   is never invoked on a half-built client.

2. **Drop**: replace `.unwrap()` / silent `let _ =` with
   `if let Err` + `log::warn!` so that errors are observable
   but never abort the application.

Fixes: https://github.com/esp-rs/esp-idf-svc/issues/653
See also: https://github.com/esp-rs/esp-idf-hal/issues/563
---
 src/ws/client.rs | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/src/ws/client.rs b/src/ws/client.rs
index 1234567..abcdefg 100644
--- a/src/ws/client.rs
+++ b/src/ws/client.rs
@@ -522,24 +522,30 @@
             return Err(EspError::from_infallible::<ESP_FAIL>().into());
         }

-        let client = Self {
-            handle,
-            timeout: t.0,
-            _callback: boxed_raw_callback,
-        };
-
-        esp!(unsafe {
+        // Complete all fallible initialization before constructing Self.
+        // This avoids triggering Drop on a partially-initialized client.
+        if let Err(e) = esp!(unsafe {
             esp_websocket_register_events(
-                client.handle,
+                handle,
                 esp_websocket_event_id_t_WEBSOCKET_EVENT_ANY,
                 Some(Self::handle),
                 unsafe_callback.as_ptr(),
             )
-        })?;
+        }) {
+            unsafe { esp_websocket_client_destroy(handle) };
+            return Err(e.into());
+        }

-        esp!(unsafe { esp_websocket_client_start(handle) })?;
+        if let Err(e) = esp!(unsafe { esp_websocket_client_start(handle) }) {
+            unsafe { esp_websocket_client_destroy(handle) };
+            return Err(e.into());
+        }

-        Ok(client)
+        Ok(Self {
+            handle,
+            timeout: t.0,
+            _callback: boxed_raw_callback,
+        })
     }

     pub fn send(&mut self, frame_type: FrameType, frame_data: &[u8]) -> Result<(), EspError> {
@@ -619,12 +625,14 @@

 impl Drop for EspWebSocketClient<'_> {
     fn drop(&mut self) {
-        if let Err(e) = esp!(unsafe { esp_websocket_client_close(self.handle, self.timeout) }) {
-            log::warn!("WebSocket close failed during drop: {e:?}");
+        // close() can return ESP_FAIL if the transport tore down asynchronously;
+        // destroy() should not fail for a fully-initialized client, but we must
+        // never panic in Drop (esp-idf targets use panic=abort).
+        if let Err(e) = esp!(unsafe { esp_websocket_client_close(self.handle, self.timeout) }) {
+            log::warn!("WebSocket close failed during drop: {e:?}");
         }

-        esp!(unsafe { esp_websocket_client_destroy(self.handle) }).unwrap();
-
-        // timeout and callback dropped automatically
+        if let Err(e) = esp!(unsafe { esp_websocket_client_destroy(self.handle) }) {
+            log::warn!("WebSocket destroy failed during drop: {e:?}");
+        }
     }
 }

Looks reasonable to me but I know literally nothing. I only could say that the crash does not reproduce with this version.