Skip to content

Commit 51e0714

Browse files
authored
Fix issues in WebSocketTask during connection (#1783) (#1785)
During the connection handshake: * `WebSocketTask::is_active` erronously returns `false`. * Dropping `WebSocketTask` drops the `WebSocket` without calling `WebSocket::close` on it, resulting in the WebSocket "leaking"; it will typically continue the connection handshake and remain connected to the server, despite the client having dropped the object representing it. Fix these by considering the task to be active if the WebSocket `ready_state` is *either* `WebSocket::OPEN` or `WebSocket::CONNECTING`. Add two tests to check that a `WebSocketTask` returns `true` for `is_active` while it's connecting, and that dropping the `WebSocketTask` while it's connecting results in the underlying WebSocket being closed properly.
1 parent a1fa79d commit 51e0714

File tree

1 file changed

+51
-1
lines changed

1 file changed

+51
-1
lines changed

packages/yew-services/src/websocket.rs

Lines changed: 51 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,10 @@ impl WebSocketTask {
245245

246246
impl Task for WebSocketTask {
247247
fn is_active(&self) -> bool {
248-
self.ws.ready_state() == WebSocket::OPEN
248+
matches!(
249+
self.ws.ready_state(),
250+
WebSocket::CONNECTING | WebSocket::OPEN
251+
)
249252
}
250253
}
251254

@@ -397,4 +400,51 @@ mod tests {
397400
),
398401
}
399402
}
403+
404+
#[test]
405+
async fn is_active_while_connecting() {
406+
let url = echo_server_url();
407+
let cb_future = CallbackFuture::<Json<Result<Message, anyhow::Error>>>::default();
408+
let callback: Callback<_> = cb_future.clone().into();
409+
let status_future = CallbackFuture::<WebSocketStatus>::default();
410+
let notification: Callback<_> = status_future.clone().into();
411+
412+
let task = WebSocketService::connect_text(url, callback, notification).unwrap();
413+
414+
// NOTE: There's a bit of a race here between checking `is_active`
415+
// and the WebSocket completing the connection handshake.
416+
// The handshake *should* take sufficient time to complete that we
417+
// can see it still in the `WebSocket::CONNECTING` state, but it's
418+
// not guaranteed. If someone has a way to guarantee we capture
419+
// the WebSocket in the connecting state, please update this test.
420+
assert!(task.is_active());
421+
422+
assert_eq!(status_future.await, WebSocketStatus::Opened);
423+
}
424+
425+
#[test]
426+
async fn drop_while_still_connecting() {
427+
let url = echo_server_url();
428+
let cb_future = CallbackFuture::<Json<Result<Message, anyhow::Error>>>::default();
429+
let callback: Callback<_> = cb_future.clone().into();
430+
let status_future = CallbackFuture::<WebSocketStatus>::default();
431+
let notification: Callback<_> = status_future.clone().into();
432+
433+
let task = WebSocketService::connect_text(url, callback, notification).unwrap();
434+
let ws = task.ws.clone();
435+
436+
// NOTE: There's a bit of a race here between dropping the
437+
// `WebSocketTask` and the WebSocket completing the connection
438+
// handshake. The handshake *should* take sufficient time to complete
439+
// that we can see it still in the `WebSocket::CONNECTING` state, but
440+
// it's not guaranteed. If someone has a way to guarantee we capture
441+
// the WebSocket in the connecting state, please update this test.
442+
drop(task);
443+
444+
let ws_ready_state = ws.ready_state();
445+
assert!(matches!(
446+
ws_ready_state,
447+
WebSocket::CLOSING | WebSocket::CLOSED
448+
));
449+
}
400450
}

0 commit comments

Comments
 (0)