Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1180.
A
Socketwhose initial connection attempt fails can never be reconnected afterwards. Settingconnected = falsethenconnected = truedoes nothing, no further connection is attempted, and no furthererroris 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
onSocketErrorlogs and re-emits but never discards the failed socket, whileonSocketDisconnected— the only placesocketis set back tonullptr— cannot run, because a socket that never connected does not emitdisconnected.I confirmed that against Qt directly rather than assuming it. Calling
connectToServer()on a missing path emitserrorOccurred(ServerNotFoundError)synchronously, before returning, and never emitsdisconnected; callingdisconnectFromServer()on the resulting socket emits nothing at all, even after an event loop spin, leaving it inUnconnectedState.So after a failed connect the object sits at
socket != nullptr(stale),connected == false,targetConnected == true,disconnecting == false, and every exit is closed:setConnected(false)takes thesocket != nullptr && !disconnectingbranch, setsdisconnecting = trueand callssocket->disconnectFromServer(), which is a no-op here — soonSocketDisconnectednever runs anddisconnectingstaystrue.setPath(...)returns early when the path is unchanged.setConnected(true)is guarded byelse if (this->socket == nullptr), now false. Nothing happens.The stale
QLocalSocketis never cleared because onlyonSocketDisconnectedclears 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
onSocketErrorwhen it is not connected, leaving the object able to dial again.Notes on the shape of it:
!this->connectedguard matters.errorOccurredalso fires on a live connection, and those must keep going throughonSocketDisconnected, which clears the buffer and emitsconnectionStateChanged. Only sockets that never connected are reclaimed here.connectToServer, callingconnectPathSocket()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.connectionStateChangedis deliberately not emitted, asconnectedwas alreadyfalseand a notify would be spurious.disconnectbeforedeleteLaterfollows 92cb890.Happy to reshape this — for instance factoring the teardown into a helper shared with
onSocketDisconnected— if you would prefer it that way.Testing
cteston this branch: 8 of 9 pass.popupwindow(moveWithParent) fails, but it fails identically on unmodifiedmasterin my environment, which is offscreen (QT_QPA_PLATFORM=offscreen) — I rebuilt with onlysrc/io/socket.cppreverted to confirm it is pre-existing and unrelated.clang-formatreports 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 aQLocalServer-based one felt like a separate unit of work. Glad to add it here or in a follow-up if you want it.