Windows: Handle QLocalSocket behavior gracefully
ClosedPublic

Authored by kfunk on Apr 28 2020, 7:14 PM.

Details

Summary

On Windows the underlying implementation of QLocalSocket behaves
differently, it seems to trigger the QLocalSocket::readyRead() signal
earlier than anticipated (as on Linux).
Prior to this patch this was not handled gracefully.

Kudos to David Faure for actually figuring this out by just looking at
the code and my debug output on Windows. :)

BEFORE:
Let's have a look at what happens on Windows inside Akonadi's Conneciton
class:

  • Akonadi data sends Hello cmd => 91 bytes
  • Akonadi resource receives data
  • QLocalSocket::readyReady() signal fires
  • Triggers Connection::handleIncomingData via signal-slot connection
  • On Windows, QLocalSocket::bytesAvailable() is just 8 bytes
  • Only the first 8 bytes are read, into the qint64 tag variable
  • Protocol::deserialize(...) is called
    • Internally calls waitForData(...) (=> waitForReadyRead()) a few times
  • QLocalSocket::readyRead() signal fires again
  • Connection::handleIncomingData re-entered => BUG

Problematic end result:

[10972] org.kde.pim.akonadicore: tag: -1099511627647
[10972] org.kde.pim.akonadicore: Invalid command, the world is going to end!
[10972] org.kde.pim.akonadicore: State changed: QLocalSocket::ClosingState
[10972] org.kde.pim.akonadicore: State changed: QLocalSocket::UnconnectedState

> Resource attempts reconnection to server, just to fail again

afterwards.

AFTER:
The fix is to temporarily disconnect from the readyRead() signal while
attempting to wait for data to deserialize commands. This in order to never
re-enter Connection::handleIncomingData() while doing so.

Diff Detail

Repository
R165 Akonadi
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kfunk created this revision.Apr 28 2020, 7:14 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptApr 28 2020, 7:14 PM
Restricted Application added a subscriber: kde-pim. · View Herald Transcript
kfunk requested review of this revision.Apr 28 2020, 7:14 PM

+1 from me (not much surprise given that I wrote version 1 of this patch) ;)

Let's see what Dan says.

kfunk added a comment.Apr 29 2020, 9:03 AM

Uhm, sorry, totally forgot to mention that you actually came up with that idea to begin with. Will add that when pushing!

kfunk updated this revision to Diff 81494.Apr 29 2020, 10:01 AM

Fix typos, add kudos for David ;)

QSignalBlocker ?

Thought about it earlier, but then we'd also ignore all the other signals from QLocalSocket we're listening to while waiting for data, i.e. QLocalSocket::errorOccurred(), etc.. That's not wanted.

And even readyRead() itself shouldn't be blocked, we *want* it to be emitted on Windows, for the QEventLoop to exit.

kfunk updated this revision to Diff 81498.Apr 29 2020, 11:34 AM
kfunk edited the summary of this revision. (Show Details)
kfunk removed a subscriber: anthonyfieroni.

Update Diff text, too

dvratil accepted this revision.Apr 30 2020, 9:08 AM
dvratil added a subscriber: dvratil.

Thanks for the patch. Land it to the release/20.04 branch, please.

This revision is now accepted and ready to land.Apr 30 2020, 9:08 AM
This revision was automatically updated to reflect the committed changes.