Replace TCPThread with RAII ConnectionManager + Incoming/Outgoing threads - #413
Open
ProfessorTom wants to merge 333 commits into
Open
Replace TCPThread with RAII ConnectionManager + Incoming/Outgoing threads#413ProfessorTom wants to merge 333 commits into
ProfessorTom wants to merge 333 commits into
Conversation
ProfessorTom
force-pushed
the
tomas/connection-raii-initial
branch
2 times, most recently
from
March 7, 2026 01:44
81ffa3c to
c4d8576
Compare
ProfessorTom
force-pushed
the
tomas/connection-raii-initial
branch
from
May 25, 2026 22:17
1c8340b to
99d2b79
Compare
ProfessorTom
force-pushed
the
tomas/connection-raii-initial
branch
from
June 30, 2026 23:34
923780f to
e2560b0
Compare
ProfessorTom
force-pushed
the
tomas/connection-raii-initial
branch
from
August 9, 2026 19:53
2a544d1 to
76673e1
Compare
Mainly for test injection / mocking. - New constructor: TCPThread(QSslSocket*, host, port, initialPacket, parent) - Move socket parenting + signal wiring into constructor - Set sendPacket.toIP/port from constructor args
give ourselves some conceptual toString()-like magic
...before we start replacing existing code in `run()` with code we pulled out into our smaller methods.
…dshake()` for consistency with upcoming naming scheme.
…or handling - Added getSslErrors() and getSslHandshakeErrors() as virtual methods - These allow TestTcpThreadClass to override and return mocked error lists - Uses const_cast internally to work around Qt's non-const sslErrors() API
- Override the new virtual helpers so MockSslSocket can supply error lists - Enables proper testing of the SSL error emission path in handleIncomingSSLHandshake()
- Moved all incoming/server-side SSL handling (loadSSLCerts, startServerEncryption, waitForEncrypted, error handling, and certificate/cipher info emission) from run() into a dedicated method handleIncomingSSLHandshake(QSslSocket &sock) - Made the method virtual to support mocking in unit tests
replaced with a comment explaining that there is abstraction that does what the TODO used to ask us to do
Allows validating packets from const contexts (e.g. `OutgoingTcpConnection::send`).
ThreadedTCPServer no longer emits packetReceived/toStatusBar/packetSent; logging uses setupConnectionLogging.
…Thread` "Connected and idle." is a healthy persistent session, not Error. Helper moves the raw socket onto the worker thread before start.
Drain enqueued packets each iteration and before close. Idle path waits on the socket instead of spinning when there is nothing to read.
Validate packet, enqueue when the worker is still running and persistent, otherwise create a new OutgoingTcpThread. Call moveSocketToWorkerThread before start on the create path.
…ring Persistent TCP opens PersistentConnection with initWithConnection and PersistentConnectionWiring; one-shot uses createOutgoingTcpConnection. Resend timer only fires when reSendPacket has repeat and a non-empty toIP.
ProfessorTom
marked this pull request as ready for review
August 13, 2026 04:13
Contributor
Author
|
@dannagle I believe this PR is finally ready for review. Thank you for giving me the opportunity to do this work. |
Add an offscreen ctest step before debuild, install translations and GL build deps, and force cmake in debian/rules so the package build matches the CMake-based project.
Use qt_zh_CN/qtbase_zh_CN (case-sensitive on Linux). Add translations.qrc to packetsender_unittests so app catalogs load in CI.
`QCOMPARE` does not accept `std::vector` vs a braced initializer list the way it does for QList. Use an explicit vector or `QVERIFY(empty())` so the tests build on Ubuntu as well as macOS. Also switch unit tests from `#include <QtTest/QTest.h>` to `#include <QTest>` so they compile on Linux.
ProfessorTom
force-pushed
the
tomas/connection-raii-initial
branch
from
August 15, 2026 18:31
44220c7 to
ca12873
Compare
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.
Before submitting a pull request:
This will likely be a Cthulhu PR that will need to be broken up into a few smaller PRs once the work is done. Unfortunately, the way through is a long running branch in the short term.
Some benefits and thoughts as work continues on this branch:
TcpThreadclass will be replaced withBaseTcpThread,OutgoingTcpThreadandIncomingTcpThread.The idea here is to combine common code in the base class but separate the different directions so that they have room to breath and you can follow the main flows more easily. All three of those classes will automagically clean up both the socket and the threads via RAII and Qt's object lifecycle and parenting system.
There will be a root
Connectionclass, aBaseTcpConnectionclass, anIncomingTCPConnectionand anOutgoingTcpConnectionclass.Here's the basic idea:
Connection (abstract)
│
├── BaseTcpConnection
│ ├── OutgoingTcpConnection
│ └── IncomingTcpConnection
│
├── BaseDtlsConnection
│ ├── OutgoingDtlsConnection
│ └── IncomingDtlsConnection
│
├── BaseUdpConnection
│ └── UdpConnection (usually no separate in/out)
│
└── ... (future protocols: RS-232 [aka Serial] QUIC, WebSocket, SCTP, etc.)
Unit tests have been added for code I have modified or added and will continue to be added for code I modify or add.
Connection state machine
BaseTcpConnectionkeeps a small explicit state (Created→Active/Inactive/Error/Closing, etc.) updated from the status strings the worker threads emit (connectionStatus). Not every status string we emit is mapped yet—CLion only shows a handful of distinct messages in practice, and several human-readable variants still fall through to a default. That’s intentional for now: the machine is mostly documentation and a single place to reason about lifecycle, not a full protocol state chart. The one place it earns its keep is when we decide whether a connection is still usable for send-queue traffic (e.g. reuse / drain on a live persistent session versus tearing down and creating a new worker). Refining the remaining status → state mappings is straightforward follow-up if we want stricter checks later.qMake is going away.
I'm not sure how Dan builds for each platform.
I was under the impression that most if not all platforms were using CMake now. I know snapcraft and macOS does. Regardless, the unit test only work with CMake, in part, because I am developing on a Mac. There may be some way to get the unit tests to build and run with qMake, but as I understand it, Dan wanted to move away from qMake anyway, so now's the chance.
This means I should be able to delete the .pro files in this PR.
At the time of this writing I am not doing so because I don't know what the consequences are for taking that action. e.g. there may be one build that is still using qMake.I talked with @dannagle and he said that Windows stills needs to be converted to CMake. I know that the snapcraft build uses CMake.Not sure about the Debian build in the pipeline.The "Debian" build that's in the pipeline is not a true Debian build: it uses an Ubuntu container to build the code and then create the.debpackage.I was able to convert the "Debian" build on GitHub to use CMake. This also means running the unit tests, which now pass on both macOS and Ubuntu. I don't have access to a Windows system, so I'm not sure what it would take to get Windows using CMake and if there will need to be any modifications to the unit tests.
Given we are dealing with some low-level functionality (e.g. IPv4 vs IPv6, os default network stacks, etc.), it's possible that some unit tests may fail on Windows. (Presumably snapcraft will build and run, though we may need to modify the script to get snapcraft running the unit tests.)
Fixes #151