Skip to content

Tomas/connection raii initial#413

Draft
ProfessorTom wants to merge 229 commits into
dannagle:developmentfrom
ProfessorTom:tomas/connection-raii-initial
Draft

Tomas/connection raii initial#413
ProfessorTom wants to merge 229 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.

qMake is likely 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.

This likely means modifying the build process to build and run the unit tests and perhaps have such failures block PR merges. While the latter can be done once here on GitHub, the former will have to be done on each platform that PacketSender is built for. 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 platforms other than macOS. I haven't tested on any other platform. I fully intend for this to be a problem for Dan to solve, not because I'm being lazy or want to give Dan more work, but because at the time of this writing, this PR is not complete and I don't have access to each build system to test them.

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
- New ctor: TCPThread(host, port, initialPacket, parent)
- Creates QSslSocket early
- Connects connected/errorOccurred/stateChanged signals
- Stores host/port for run()
- Sets m_managedByConnection = true
- [[nodiscard]] bool isValid() const
- Checks clientConnection null, error codes, socket state
- Adds detailed qDebug/qWarning logging for failure reasons
- Add null check for clientConnection before calling close()
- Add waitForDisconnected(1000) for clean shutdown when socket exists
- Log warning if called with null socket
- Prevents potential segfault in dtor when socket not initialized
- Add if (clientConnection) guard before close()
- Only call waitForDisconnected() if socket is not UnconnectedState or ClosingState
- Log when wait is skipped or socket is null
- Eliminates Qt warning about waitForDisconnected in UnconnectedState
- Prevents potential null dereference in Connection dtor
…safe cleanup

- Update ctor to pass host/port/initialPacket
- Add public start() method with isValid() check
- Auto-start in ctor
- Add send(), isConnected(), isSecure()
- Forward key TCPThread signals
- Add m_threadStarted flag for safe dtor cleanup
- Add testThreadStartsAndStops() to verify no crash on start/dtor
- Give thread time to start with QTest::qWait(500)
- Check isConnected() (with fallback for Connecting state)
…plication

- Link packet.cpp, tcpthread.cpp, sendpacketbutton.cpp
- Add Qt6::Network for QSslSocket/QTcpSocket
- Switch to per test class QApplication isolation with runGuiTest/runNonGuiTest
- Added Q_OBJECT
- Added signals: shutdown, destructor, runStarted
- Improved shutdown(), stop(), and run() overrides with recording and signals
- Added thread ID capture in run()
- Tests that send() properly shuts down old thread and creates new one
- Uses signals and thread ID comparison for verification
…ingTcpThread

- Removed confusing "Initial" prefix from method name
- Updated implementation, header, test double, and all references
- Updated corresponding CallTracker constant
- Cleaned up related unit tests
- Added virtual `isInterruptionRequested()` method to BaseTcpThread
- Implemented using QThread::isInterruptionRequested()
- Added full test double support in IncomingTcpThreadTestDouble
  (call recording + controllable return value for tests)
- Improves testability of thread interruption logic
- Added `persistentConnectionLoop()` that continuously waits for data,
  builds received packets, emits `packetReceived`, and sends smart replies
- Made method virtual so test doubles can override it
- Added comprehensive unit tests covering null socket, success path,
  and packet emission
- Enhanced IncomingTcpThreadTestDouble with controllable loop iterations
  and call recording
- Updated CallTracker constants
- Added protected `terminateConnection()` that shuts down the thread and emits disconnected() if needed
- Added public `close()` that delegates to terminateConnection()
- Updated BaseTcpThread destructor to call shutdown()
- Added comprehensive termination tests
- Updated IncomingTcpThreadTestDouble with shutdown signal for testing
- Minor ConnectionManager and CMake cleanup
- Added `createIncomingTcpConnection()` and `createOutgoingTcpConnection()`
  that return std::pair<quint64, IncomingTcpConnection*> and std::pair<quint64, OutgoingTcpConnection*> respectively
- Added ConnectionManagerTestDouble to expose internal state for testing
- Updated unit tests for the new factory methods
- Enabled ConnectionManagerTests in test runner
- Added pure virtual receiveData() to Connection
- Added default implementation in BaseTcpConnection (throws unsupported)
- Added receiveData() that creates and starts IncomingTcpThread
- Added makeIncomingTcpThread() factory method
- Updated IncomingTcpConnectionTestDouble
- Added test_receiveData_threadDoesNotExist() and test_receiveData_replacesExistingThread()
- Added `disconnected()` signal emitted in closeConnection()
- Added testCloseConnection_emitsDisconnectedSignal()
- Added virtual setupSignalConnections() in Connection base
- Implemented signal wiring in BaseTcpConnection
- Updated IncomingTcpConnection and OutgoingTcpConnection to call it
- Updated test doubles to record the call
- Added verification in connection tests
- Added setupConnectionSignals() to wire Connection signals to manager with ID prefix
- Updated create*Connection() methods to call setupConnectionSignals()
- Added tests for signal setup
- Updated ConnectionManagerTestDouble to record the call
@ProfessorTom ProfessorTom force-pushed the tomas/connection-raii-initial branch from 923780f to e2560b0 Compare June 30, 2026 23:34
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