Skip to content

fix(ws): do not panic in EspWebSocketClient::drop on destroy failure - #657

Open
SAY-5 wants to merge 1 commit into
esp-rs:masterfrom
SAY-5:fix/ws-drop-destroy-no-panic
Open

fix(ws): do not panic in EspWebSocketClient::drop on destroy failure#657
SAY-5 wants to merge 1 commit into
esp-rs:masterfrom
SAY-5:fix/ws-drop-destroy-no-panic

Conversation

@SAY-5

@SAY-5 SAY-5 commented Apr 17, 2026

Copy link
Copy Markdown

Problem

#648 fixed the unwrap() on esp_websocket_client_close inside
EspWebSocketClient::drop, but left the matching call to
esp_websocket_client_destroy as:

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
    }
}

A Drop impl must not panic. On ESP-IDF targets built with
panic = "abort" any panic in Drop aborts the whole application
without unwinding, so this is worse than a missed error.

The failure is reachable in practice: new_raw constructs the client
before calling esp_websocket_client_start(), so a failed start returns
Err via ? and drops a client whose transport was never established,
which is exactly the scenario where destroy can fail. Reported in
#653.

Fix

Apply the same treatment already used for close: log the error and
swallow it. Same warning style so log noise stays consistent.

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

Fixes #653

esp-rs#648 fixed the unwrap on esp_websocket_client_close inside Drop, but left
the matching call to esp_websocket_client_destroy as

  esp!(unsafe { esp_websocket_client_destroy(self.handle) }).unwrap();

A Drop impl must not panic. On ESP-IDF targets built with panic = "abort"
any panic in Drop aborts the whole application without unwinding. The
failure is reachable in practice: new_raw constructs the client before
calling esp_websocket_client_start(), so a failed start returns Err and
drops a client whose transport was never established, which can make the
destroy call fail.

Apply the same treatment as the close call: log the error and swallow it.

Fixes esp-rs#653

Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
@SAY-5
SAY-5 force-pushed the fix/ws-drop-destroy-no-panic branch from 6901c11 to 95be340 Compare May 31, 2026 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

EspWebSocketClient::Drop still panics: esp_websocket_client_destroy unwrap

1 participant