Skip to content

io/socket: discard failed sockets so reconnection works - #1181

Open
ayandexyz wants to merge 1 commit into
quickshell-mirror:masterfrom
ayandexyz:fix/socket-reconnect-after-failed-connect
Open

ayandexyz wants to merge 1 commit into
quickshell-mirror:masterfrom
ayandexyz:fix/socket-reconnect-after-failed-connect

Conversation

@ayandexyz

Copy link
Copy Markdown

Fixes #1180.

A Socket whose initial connection attempt fails can never be reconnected afterwards. Setting connected = false then connected = true does nothing, no further connection is attempted, and no further error is emitted — the object is silently wedged for the rest of its life.

This bites any shell subscribing to a socket owned by a daemon that may not be up yet at shell start. In my case the shell and the daemon are both started by the graphical session with no ordering between them, so losing the race by one second left a bar widget reporting "daemon not running" for eight hours while the daemon was perfectly healthy.

Cause

onSocketError logs and re-emits but never discards the failed socket, while onSocketDisconnected — the only place socket is set back to nullptr — cannot run, because a socket that never connected does not emit disconnected.

I confirmed that against Qt directly rather than assuming it. Calling connectToServer() on a missing path emits errorOccurred(ServerNotFoundError) synchronously, before returning, and never emits disconnected; calling disconnectFromServer() on the resulting socket emits nothing at all, even after an event loop spin, leaving it in UnconnectedState.

So after a failed connect the object sits at socket != nullptr (stale), connected == false, targetConnected == true, disconnecting == false, and every exit is closed:

  1. setConnected(false) takes the socket != nullptr && !disconnecting branch, sets disconnecting = true and calls socket->disconnectFromServer(), which is a no-op here — so onSocketDisconnected never runs and disconnecting stays true.
  2. setPath(...) returns early when the path is unchanged.
  3. setConnected(true) is guarded by else if (this->socket == nullptr), now false. Nothing happens.

The stale QLocalSocket is never cleared because only onSocketDisconnected clears it, and it cannot fire. This also explains why the failure is so quiet: exactly one warning is ever logged, because no subsequent connection is attempted and so nothing further can fail.

Change

Discard the socket from onSocketError when it is not connected, leaving the object able to dial again.

Notes on the shape of it:

  • The !this->connected guard matters. errorOccurred also fires on a live connection, and those must keep going through onSocketDisconnected, which clears the buffer and emits connectionStateChanged. Only sockets that never connected are reclaimed here.
  • No reconnect is attempted from this path. Because the error is emitted synchronously from inside connectToServer, calling connectPathSocket() here would recurse for as long as the path stays unavailable. Leaving the object re-dialable lets the QML side drive its own retry, which is what the property docs already describe.
  • connectionStateChanged is deliberately not emitted, as connected was already false and a notify would be spurious.
  • disconnect before deleteLater follows 92cb890.

Happy to reshape this — for instance factoring the teardown into a helper shared with onSocketDisconnected — if you would prefer it that way.

Testing

  • ctest on this branch: 8 of 9 pass. popupwindow (moveWithParent) fails, but it fails identically on unmodified master in my environment, which is offscreen (QT_QPA_PLATFORM=offscreen) — I rebuilt with only src/io/socket.cpp reverted to confirm it is pre-existing and unrelated.
  • The reproducer from Socket cannot reconnect after a failed initial connection (onSocketError never clears the stale socket) #1180, run against a build of this branch, now connects on the first retry and streams data. Before the change it knocked forever without ever connecting.
  • clang-format reports no changes.

Build configured with -DBUILD_TESTING=ON, with the Wayland/X11/service features disabled, as this machine lacks some of their dependencies.

I have not added a regression test — src/io/test/ has no socket harness yet, and adding a QLocalServer-based one felt like a separate unit of work. Glad to add it here or in a follow-up if you want it.

A QLocalSocket whose connection attempt fails emits errorOccurred but
never emits disconnected, so onSocketDisconnected does not run and the
failed socket is kept in place. Both setConnected and setPath only dial
when socket is null, so every later connection attempt becomes a no-op
and the Socket can never be reconnected.

Discard the socket from onSocketError when it is not connected, leaving
the object able to dial again. No reconnect is attempted from here, as
errorOccurred is emitted synchronously from within connectToServer and
retrying would recurse for as long as the path stays unavailable.
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.

Socket cannot reconnect after a failed initial connection (onSocketError never clears the stale socket)

1 participant