Skip to content

Websocket server session ends when a client connects without completing the handshake #944

Description

@GuillaumeDua

Describe the bug

ButtplugWebsocketServerTransport::connect accepts one connection and propagates a failed upgrade out of the whole future (crates/buttplug_transport_websocket_tungstenite/src/websocket_server.rs:280):

if let Ok((stream, _)) = listener.accept().await {
  info!("Websocket: Got connection");
  let ws_stream = tokio_tungstenite::accept_async(stream)
    .await
    .map_err(|err| { /* ... */ })?;

So any TCP client that connects and goes away without completing the HTTP upgrade ends the server session.
The FapTap web client does exactly that: it opens a probe connection, then opens the real websocket 1.35 to
9.12 s later. Repro without a specific client: connect a socket to the server port and close it without
sending anything.

Expected behavior

A connection that fails the upgrade gets logged and ignored, and the listener keeps waiting for a real client.

Actual behavior

The session dies and the engine surfaces an error to the user:

72.12  [I] Websocket: Got connection
72.121 [E] Websocket server accept error: Protocol(HandshakeIncomplete)
72.121 [E] Engine error: Process Error: ConnectorError(TransportSpecificError(GenericNetworkError("Protocol(HandshakeIncomplete)")))
72.125 [D] Websocket: Trying to listen on 0.0.0.0:12345
81.223 [I] Websocket: Got connection
81.24  [I] Performing server handshake check with client FapTap at message version Version3.0

Six times in one session, no exception.
Intiface Central rebinds about 4 ms later and nothing is lost, so in practice this is a spurious error banner rather than a broken connection.
Seen with buttplug 11.0.0 through Intiface Central 3.1.1+43 on Windows 11 - code read at master 9571b3db.

Additional context

The fix is to loop on accept() and continue after a warn! when the upgrade fails.
The transport stays single connection either way: outgoing_receiver is moved into the spawned connection loop, so the loop can only run until the first successful handshake.

Two calls I'd rather have from you before writing it:

  • should an Err from listener.accept() itself be retried too, or keep returning ConnectorGenericError as it does today ?
  • should the accept loop select! on disconnect_notifier ?
    Today a failed handshake ends the future, which is the bug, but it also happens to end a session that gets disconnected before any client connects.
    With the loop, that future waits forever unless disconnect is observed.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions