Tomas/connection raii initial#413
Draft
ProfessorTom wants to merge 229 commits into
Draft
Conversation
81ffa3c to
c4d8576
Compare
1c8340b to
99d2b79
Compare
…tart unit testing
- 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
…rt or end with a given sub-vector
- 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
923780f to
e2560b0
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.
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