From b14f5f59a3ae96949e6a33302385a751d6448182 Mon Sep 17 00:00:00 2001 From: Ryan Chu Date: Fri, 28 Jun 2019 14:37:05 +0200 Subject: Remove waitForReadyRead from QWebSocketFrame::readFrame MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Asynchronously process socket frame in QWebSocketDataProcessor::process. If the processing of QWebSocketFrame is not done and waiting for more data, QWebSocketDataProcessor::process will return the control and wait for next readyRead signal to continue processing the unfinished socket frame. QWebSocketDataProcessor::process gets timeout after 5 seconds, and then an errorEncountered signal will be emitted. Fixes: QTBUG-74464 Change-Id: I03b7f874c1c266617e7eadf59c59ae43fa8540ce Reviewed-by: Edward Welbourne Reviewed-by: Mårten Nordheim --- src/websockets/qwebsocket_p.cpp | 4 +- src/websockets/qwebsocketdataprocessor.cpp | 22 +++++++- src/websockets/qwebsocketdataprocessor_p.h | 3 ++ src/websockets/qwebsocketframe.cpp | 62 ++++++++++------------ src/websockets/qwebsocketframe_p.h | 1 + .../websockets/dataprocessor/tst_dataprocessor.cpp | 16 ++++-- .../auto/websockets/qwebsocket/tst_qwebsocket.cpp | 36 ++++++++++++- 7 files changed, 103 insertions(+), 41 deletions(-) diff --git a/src/websockets/qwebsocket_p.cpp b/src/websockets/qwebsocket_p.cpp index 6965731..9b94d04 100644 --- a/src/websockets/qwebsocket_p.cpp +++ b/src/websockets/qwebsocket_p.cpp @@ -1173,10 +1173,10 @@ void QWebSocketPrivate::processData() { if (!m_pSocket) // disconnected with data still in-bound return; - while (m_pSocket->bytesAvailable()) { + if (m_pSocket->bytesAvailable()) { if (state() == QAbstractSocket::ConnectingState) { if (!m_pSocket->canReadLine()) - break; + return; processHandshake(m_pSocket); } else { m_dataProcessor.process(m_pSocket); diff --git a/src/websockets/qwebsocketdataprocessor.cpp b/src/websockets/qwebsocketdataprocessor.cpp index ee28969..39287d4 100644 --- a/src/websockets/qwebsocketdataprocessor.cpp +++ b/src/websockets/qwebsocketdataprocessor.cpp @@ -87,6 +87,10 @@ QWebSocketDataProcessor::QWebSocketDataProcessor(QObject *parent) : m_pTextCodec(QTextCodec::codecForName("UTF-8")) { clear(); + // initialize the internal timeout timer + waitTimer.setInterval(5000); + waitTimer.setSingleShot(true); + waitTimer.callOnTimeout(this, &QWebSocketDataProcessor::timeout); } /*! @@ -126,7 +130,13 @@ void QWebSocketDataProcessor::process(QIODevice *pIoDevice) while (!isDone) { frame.readFrame(pIoDevice); - if (Q_LIKELY(frame.isValid())) { + if (!frame.isDone()) { + // waiting for more data available + QObject::connect(pIoDevice, &QIODevice::readyRead, + &waitTimer, &QTimer::stop, Qt::UniqueConnection); + waitTimer.start(); + return; + } else if (Q_LIKELY(frame.isValid())) { if (frame.isControlFrame()) { isDone = processControlFrame(frame); } else { @@ -302,4 +312,14 @@ bool QWebSocketDataProcessor::processControlFrame(const QWebSocketFrame &frame) return mustStopProcessing; } +/*! + \internal + */ +void QWebSocketDataProcessor::timeout() +{ + clear(); + Q_EMIT errorEncountered(QWebSocketProtocol::CloseCodeGoingAway, + tr("Timeout when reading data from socket.")); +} + QT_END_NAMESPACE diff --git a/src/websockets/qwebsocketdataprocessor_p.h b/src/websockets/qwebsocketdataprocessor_p.h index 1d8024e..41226d6 100644 --- a/src/websockets/qwebsocketdataprocessor_p.h +++ b/src/websockets/qwebsocketdataprocessor_p.h @@ -55,6 +55,7 @@ #include #include #include +#include #include "qwebsocketframe_p.h" #include "qwebsocketprotocol.h" #include "qwebsocketprotocol_p.h" @@ -113,8 +114,10 @@ private: QTextCodec::ConverterState *m_pConverterState; QTextCodec *m_pTextCodec; QWebSocketFrame frame; + QTimer waitTimer; bool processControlFrame(const QWebSocketFrame &frame); + void timeout(); }; QT_END_NAMESPACE diff --git a/src/websockets/qwebsocketframe.cpp b/src/websockets/qwebsocketframe.cpp index 5b10e02..11373a7 100644 --- a/src/websockets/qwebsocketframe.cpp +++ b/src/websockets/qwebsocketframe.cpp @@ -188,7 +188,7 @@ void QWebSocketFrame::swap(QWebSocketFrame &other) */ QWebSocketProtocol::CloseCode QWebSocketFrame::closeCode() const { - return m_closeCode; + return isDone() ? m_closeCode : QWebSocketProtocol::CloseCodeGoingAway; } /*! @@ -196,7 +196,7 @@ QWebSocketProtocol::CloseCode QWebSocketFrame::closeCode() const */ QString QWebSocketFrame::closeReason() const { - return m_closeReason; + return isDone() ? m_closeReason : tr("Waiting for more data from socket."); } /*! @@ -289,67 +289,63 @@ void QWebSocketFrame::clear() */ bool QWebSocketFrame::isValid() const { - return m_isValid; + return isDone() && m_isValid; } -#define WAIT_FOR_MORE_DATA(returnState) \ - { needMoreData = true; \ - m_processingState = (returnState); } +/*! + \internal + */ +bool QWebSocketFrame::isDone() const +{ + return m_processingState == PS_DISPATCH_RESULT; +} /*! \internal */ void QWebSocketFrame::readFrame(QIODevice *pIoDevice) { - bool isDone = false; - while (!isDone) + while (true) { - bool needMoreData = false; switch (m_processingState) { case PS_READ_HEADER: m_processingState = readFrameHeader(pIoDevice); - if (m_processingState == PS_WAIT_FOR_MORE_DATA) - WAIT_FOR_MORE_DATA(PS_READ_HEADER); + if (m_processingState == PS_WAIT_FOR_MORE_DATA) { + m_processingState = PS_READ_HEADER; + return; + } break; case PS_READ_PAYLOAD_LENGTH: m_processingState = readFramePayloadLength(pIoDevice); - if (m_processingState == PS_WAIT_FOR_MORE_DATA) - WAIT_FOR_MORE_DATA(PS_READ_PAYLOAD_LENGTH); + if (m_processingState == PS_WAIT_FOR_MORE_DATA) { + m_processingState = PS_READ_PAYLOAD_LENGTH; + return; + } break; case PS_READ_MASK: m_processingState = readFrameMask(pIoDevice); - if (m_processingState == PS_WAIT_FOR_MORE_DATA) - WAIT_FOR_MORE_DATA(PS_READ_MASK); + if (m_processingState == PS_WAIT_FOR_MORE_DATA) { + m_processingState = PS_READ_MASK; + return; + } break; case PS_READ_PAYLOAD: m_processingState = readFramePayload(pIoDevice); - if (m_processingState == PS_WAIT_FOR_MORE_DATA) - WAIT_FOR_MORE_DATA(PS_READ_PAYLOAD); + if (m_processingState == PS_WAIT_FOR_MORE_DATA) { + m_processingState = PS_READ_PAYLOAD; + return; + } break; case PS_DISPATCH_RESULT: - m_processingState = PS_DISPATCH_RESULT; - isDone = true; - break; + return; default: Q_UNREACHABLE(); - break; - } - - if (needMoreData) { - // TODO: waitForReadyRead should really be changed - // now, when a WebSocket is used in a GUI thread - // the GUI will hang for at most 5 seconds - // maybe, a QStateMachine should be used - if (!pIoDevice->waitForReadyRead(5000)) { - setError(QWebSocketProtocol::CloseCodeGoingAway, - tr("Timeout when reading data from socket.")); - m_processingState = PS_DISPATCH_RESULT; - } + return; } } } diff --git a/src/websockets/qwebsocketframe_p.h b/src/websockets/qwebsocketframe_p.h index be79a6e..e2b4e9f 100644 --- a/src/websockets/qwebsocketframe_p.h +++ b/src/websockets/qwebsocketframe_p.h @@ -101,6 +101,7 @@ public: void clear(); bool isValid() const; + bool isDone() const; void readFrame(QIODevice *pIoDevice); diff --git a/tests/auto/websockets/dataprocessor/tst_dataprocessor.cpp b/tests/auto/websockets/dataprocessor/tst_dataprocessor.cpp index f9a91d5..705ea39 100644 --- a/tests/auto/websockets/dataprocessor/tst_dataprocessor.cpp +++ b/tests/auto/websockets/dataprocessor/tst_dataprocessor.cpp @@ -193,7 +193,7 @@ private: //sequences void nonCharacterSequence(const char *sequence); - void doTest(); + void doTest(int timeout = 0); void doCloseFrameTest(); QString opCodeToString(quint8 opCode); @@ -744,6 +744,7 @@ void tst_DataProcessor::frameTooSmall() dataProcessor.process(&buffer); + QTRY_VERIFY_WITH_TIMEOUT(errorSpy.count(), 7000); QCOMPARE(errorSpy.count(), 1); QCOMPARE(closeSpy.count(), 0); QCOMPARE(pingMessageSpy.count(), 0); @@ -776,6 +777,7 @@ void tst_DataProcessor::frameTooSmall() dataProcessor.process(&buffer); + QTRY_VERIFY_WITH_TIMEOUT(errorSpy.count(), 7000); QCOMPARE(errorSpy.count(), 1); QCOMPARE(closeSpy.count(), 0); QCOMPARE(pingMessageSpy.count(), 0); @@ -808,6 +810,8 @@ void tst_DataProcessor::frameTooSmall() dataProcessor.process(&buffer); + QTRY_VERIFY_WITH_TIMEOUT(errorSpy.count(), 7000); + buffer.close(); data.clear(); @@ -820,6 +824,7 @@ void tst_DataProcessor::frameTooSmall() SIGNAL(errorEncountered(QWebSocketProtocol::CloseCode,QString))); dataProcessor.process(&buffer); + QTRY_VERIFY_WITH_TIMEOUT(errorSpy.count(), 7000); QCOMPARE(errorSpy.count(), 1); QCOMPARE(closeSpy.count(), 0); QCOMPARE(pingMessageSpy.count(), 0); @@ -849,6 +854,7 @@ void tst_DataProcessor::frameTooSmall() buffer.open(QIODevice::ReadOnly); dataProcessor.process(&buffer); + QTRY_VERIFY_WITH_TIMEOUT(errorSpy.count(), 7000); QCOMPARE(errorSpy.count(), 1); QCOMPARE(closeSpy.count(), 0); QCOMPARE(pingMessageSpy.count(), 0); @@ -877,6 +883,7 @@ void tst_DataProcessor::frameTooSmall() buffer.open(QIODevice::ReadOnly); dataProcessor.process(&buffer); + QTRY_VERIFY_WITH_TIMEOUT(errorSpy.count(), 7000); QCOMPARE(errorSpy.count(), 1); QCOMPARE(closeSpy.count(), 0); QCOMPARE(pingMessageSpy.count(), 0); @@ -1400,7 +1407,7 @@ void tst_DataProcessor::incompletePayload_data() void tst_DataProcessor::incompletePayload() { - doTest(); + doTest(7000); } void tst_DataProcessor::incompleteSizeField_data() @@ -1430,13 +1437,13 @@ void tst_DataProcessor::incompleteSizeField_data() void tst_DataProcessor::incompleteSizeField() { - doTest(); + doTest(7000); } ////////////////////////////////////////////////////////////////////////////////////////// /// HELPER FUNCTIONS ////////////////////////////////////////////////////////////////////////////////////////// -void tst_DataProcessor::doTest() +void tst_DataProcessor::doTest(int timeout) { QFETCH(quint8, firstByte); QFETCH(quint8, secondByte); @@ -1465,6 +1472,7 @@ void tst_DataProcessor::doTest() buffer.setData(data); buffer.open(QIODevice::ReadOnly); dataProcessor.process(&buffer); + QTRY_VERIFY_WITH_TIMEOUT(errorSpy.count(), timeout); QCOMPARE(errorSpy.count(), 1); QCOMPARE(textMessageSpy.count(), 0); QCOMPARE(binaryMessageSpy.count(), 0); diff --git a/tests/auto/websockets/qwebsocket/tst_qwebsocket.cpp b/tests/auto/websockets/qwebsocket/tst_qwebsocket.cpp index 2bb5d16..19af815 100644 --- a/tests/auto/websockets/qwebsocket/tst_qwebsocket.cpp +++ b/tests/auto/websockets/qwebsocket/tst_qwebsocket.cpp @@ -457,12 +457,46 @@ void tst_QWebSocket::tst_sendTextMessage() QVERIFY(isLastFrame); socket.close(); + socketConnectedSpy.clear(); + textMessageReceived.clear(); + textFrameReceived.clear(); - //QTBUG-36762: QWebSocket emits multiplied signals when socket was reopened + // QTBUG-74464 QWebsocket doesn't receive text (binary) message with size > 32 kb + socket.open(url); + + QTRY_COMPARE(socketConnectedSpy.count(), 1); + QCOMPARE(socketError.count(), 0); + QCOMPARE(socket.state(), QAbstractSocket::ConnectedState); + arguments = serverConnectedSpy.takeFirst(); + urlConnected = arguments.at(0).toUrl(); + QCOMPARE(urlConnected, url); + QCOMPARE(socket.bytesToWrite(), 0); + + // transmit a long text message with 64 kb + QString longString(65536, 'a'); + socket.sendTextMessage(longString); + QVERIFY(socket.bytesToWrite() > longString.length()); + QVERIFY(textMessageReceived.wait()); + QCOMPARE(socket.bytesToWrite(), 0); + + QCOMPARE(textMessageReceived.count(), 1); + QCOMPARE(binaryMessageReceived.count(), 0); + QCOMPARE(binaryFrameReceived.count(), 0); + arguments = textMessageReceived.takeFirst(); + messageReceived = arguments.at(0).toString(); + QCOMPARE(messageReceived.length(), longString.length()); + QCOMPARE(messageReceived, longString); + + arguments = textFrameReceived.takeLast(); + isLastFrame = arguments.at(1).toBool(); + QVERIFY(isLastFrame); + + socket.close(); socketConnectedSpy.clear(); textMessageReceived.clear(); textFrameReceived.clear(); + //QTBUG-36762: QWebSocket emits multiplied signals when socket was reopened socket.open(QUrl(QStringLiteral("ws://") + echoServer.hostAddress().toString() + QStringLiteral(":") + QString::number(echoServer.port()))); -- cgit v1.2.1