Skip to content

Replace TCPThread with RAII ConnectionManager + Incoming/Outgoing threads - #413

Open
ProfessorTom wants to merge 333 commits into
dannagle:developmentfrom
ProfessorTom:tomas/connection-raii-initial
Open

Replace TCPThread with RAII ConnectionManager + Incoming/Outgoing threads#413
ProfessorTom wants to merge 333 commits into
dannagle:developmentfrom
ProfessorTom:tomas/connection-raii-initial

Conversation

@ProfessorTom

@ProfessorTom ProfessorTom commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

Before submitting a pull request:

  • Did you fork from the development branch? yes
  • Are you submitting the pull request to the development branch? (not master) yes

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:

TcpThread class will be replaced with BaseTcpThread, OutgoingTcpThread and IncomingTcpThread.

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 Connection class, a BaseTcpConnection class, an IncomingTCPConnection and an OutgoingTcpConnection class.

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

BaseTcpConnection keeps a small explicit state (CreatedActive / 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 .deb package.

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

@ProfessorTom
ProfessorTom force-pushed the tomas/connection-raii-initial branch 2 times, most recently from 81ffa3c to c4d8576 Compare March 7, 2026 01:44
@ProfessorTom
ProfessorTom force-pushed the tomas/connection-raii-initial branch from 1c8340b to 99d2b79 Compare May 25, 2026 22:17
@ProfessorTom
ProfessorTom force-pushed the tomas/connection-raii-initial branch from 923780f to e2560b0 Compare June 30, 2026 23:34
@ProfessorTom
ProfessorTom force-pushed the tomas/connection-raii-initial branch from 2a544d1 to 76673e1 Compare August 9, 2026 19:53
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
ProfessorTom marked this pull request as ready for review August 13, 2026 04:13
@ProfessorTom

Copy link
Copy Markdown
Contributor Author

@dannagle I believe this PR is finally ready for review.

Thank you for giving me the opportunity to do this work.

@ProfessorTom ProfessorTom changed the title Tomas/connection raii initial Replace TCPThread with RAII ConnectionManager + Incoming/Outgoing threads Aug 13, 2026
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
ProfessorTom force-pushed the tomas/connection-raii-initial branch from 44220c7 to ca12873 Compare August 15, 2026 18:31
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.

1 participant