From eae0cb09f1310e755c2aff7c1112f7a6c09d7a53 Mon Sep 17 00:00:00 2001 From: Markus Goetz Date: Fri, 19 Jun 2015 15:35:34 +0200 Subject: [PATCH] Network: Fix up previous corruption patch This is a fix-up for cff39fba10ffc10ee4dcfdc66ff6528eb26462d3. That patch lead to some internal state issues that lead to the QTBUG-47048 or to QNetworkReply objects erroring with "Connection Closed" when the server closed the Keep-Alive connection. This patch changes the QNAM socket slot connections to be DirectConnection. We don't close the socket anymore in slots where it is anyway in a closed state afterwards. This prevents event/stack recursions. We also flush QSslSocket/QTcpSocket receive buffers when receiving a disconnect so that the developer always gets the full decrypted data from the buffers. [ChangeLog][QtNetwork] Fix HTTP issues with "Unknown Error" and "Connection Closed" [ChangeLog][QtNetwork][Sockets] Read OS/encrypted read buffers when connection closed by server. Change-Id: Ib4d6a2d0d988317e3a5356f36e8dbcee4590beed Task-number: QTBUG-47048 Reviewed-by: Kai Koehne Reviewed-by: Richard J. Moore --- src/network/access/qhttpnetworkconnection.cpp | 1 - .../access/qhttpnetworkconnectionchannel.cpp | 108 +++++++++++++-------- .../access/qhttpnetworkconnectionchannel_p.h | 1 + src/network/access/qhttpnetworkreply.cpp | 2 +- src/network/access/qhttpprotocolhandler.cpp | 1 - src/network/socket/qabstractsocket.cpp | 7 +- src/network/ssl/qsslsocket.cpp | 8 ++ src/network/ssl/qsslsocket_openssl.cpp | 7 ++ .../access/qnetworkreply/tst_qnetworkreply.cpp | 9 +- 9 files changed, 94 insertions(+), 50 deletions(-) diff --git a/src/network/access/qhttpnetworkconnection.cpp b/src/network/access/qhttpnetworkconnection.cpp index 365ce55..543c70e 100644 --- a/src/network/access/qhttpnetworkconnection.cpp +++ b/src/network/access/qhttpnetworkconnection.cpp @@ -917,7 +917,6 @@ void QHttpNetworkConnectionPrivate::_q_startNextRequest() for (int i = 0; i < channelCount; ++i) { if (channels[i].resendCurrent && (channels[i].state != QHttpNetworkConnectionChannel::ClosingState)) { channels[i].resendCurrent = false; - channels[i].state = QHttpNetworkConnectionChannel::IdleState; // if this is not possible, error will be emitted and connection terminated if (!channels[i].resetUploadData()) diff --git a/src/network/access/qhttpnetworkconnectionchannel.cpp b/src/network/access/qhttpnetworkconnectionchannel.cpp index 49c6793..e2f6307 100644 --- a/src/network/access/qhttpnetworkconnectionchannel.cpp +++ b/src/network/access/qhttpnetworkconnectionchannel.cpp @@ -58,6 +58,11 @@ QT_BEGIN_NAMESPACE // TODO: Put channel specific stuff here so it does not polute qhttpnetworkconnection.cpp +// Because in-flight when sending a request, the server might close our connection (because the persistent HTTP +// connection times out) +// We use 3 because we can get a _q_error 3 times depending on the timing: +static const int reconnectAttemptsDefault = 3; + QHttpNetworkConnectionChannel::QHttpNetworkConnectionChannel() : socket(0) , ssl(false) @@ -69,7 +74,7 @@ QHttpNetworkConnectionChannel::QHttpNetworkConnectionChannel() , resendCurrent(false) , lastStatus(0) , pendingEncrypt(false) - , reconnectAttempts(2) + , reconnectAttempts(reconnectAttemptsDefault) , authMethod(QAuthenticatorPrivate::None) , proxyAuthMethod(QAuthenticatorPrivate::None) , authenticationCredentialsSent(false) @@ -106,19 +111,18 @@ void QHttpNetworkConnectionChannel::init() socket->setProxy(QNetworkProxy::NoProxy); #endif - // We want all signals (except the interactive ones) be connected as QueuedConnection - // because else we're falling into cases where we recurse back into the socket code - // and mess up the state. Always going to the event loop (and expecting that when reading/writing) - // is safer. + // After some back and forth in all the last years, this is now a DirectConnection because otherwise + // the state inside the *Socket classes gets messed up, also in conjunction with the socket notifiers + // which behave slightly differently on Windows vs Linux QObject::connect(socket, SIGNAL(bytesWritten(qint64)), this, SLOT(_q_bytesWritten(qint64)), - Qt::QueuedConnection); + Qt::DirectConnection); QObject::connect(socket, SIGNAL(connected()), this, SLOT(_q_connected()), - Qt::QueuedConnection); + Qt::DirectConnection); QObject::connect(socket, SIGNAL(readyRead()), this, SLOT(_q_readyRead()), - Qt::QueuedConnection); + Qt::DirectConnection); // The disconnected() and error() signals may already come // while calling connectToHost(). @@ -129,10 +133,10 @@ void QHttpNetworkConnectionChannel::init() qRegisterMetaType(); QObject::connect(socket, SIGNAL(disconnected()), this, SLOT(_q_disconnected()), - Qt::QueuedConnection); + Qt::DirectConnection); QObject::connect(socket, SIGNAL(error(QAbstractSocket::SocketError)), this, SLOT(_q_error(QAbstractSocket::SocketError)), - Qt::QueuedConnection); + Qt::DirectConnection); #ifndef QT_NO_NETWORKPROXY @@ -147,13 +151,13 @@ void QHttpNetworkConnectionChannel::init() // won't be a sslSocket if encrypt is false QObject::connect(sslSocket, SIGNAL(encrypted()), this, SLOT(_q_encrypted()), - Qt::QueuedConnection); + Qt::DirectConnection); QObject::connect(sslSocket, SIGNAL(sslErrors(QList)), this, SLOT(_q_sslErrors(QList)), Qt::DirectConnection); QObject::connect(sslSocket, SIGNAL(encryptedBytesWritten(qint64)), this, SLOT(_q_encryptedBytesWritten(qint64)), - Qt::QueuedConnection); + Qt::DirectConnection); if (ignoreAllSslErrors) sslSocket->ignoreSslErrors(); @@ -397,7 +401,7 @@ void QHttpNetworkConnectionChannel::allDone() // reset the reconnection attempts after we receive a complete reply. // in case of failures, each channel will attempt two reconnects before emitting error. - reconnectAttempts = 2; + reconnectAttempts = reconnectAttemptsDefault; // now the channel can be seen as free/idle again, all signal emissions for the reply have been done if (state != QHttpNetworkConnectionChannel::ClosingState) @@ -651,6 +655,15 @@ void QHttpNetworkConnectionChannel::closeAndResendCurrentRequest() QMetaObject::invokeMethod(connection, "_q_startNextRequest", Qt::QueuedConnection); } +void QHttpNetworkConnectionChannel::resendCurrentRequest() +{ + requeueCurrentlyPipelinedRequests(); + if (reply) + resendCurrent = true; + if (qobject_cast(connection)) + QMetaObject::invokeMethod(connection, "_q_startNextRequest", Qt::QueuedConnection); +} + bool QHttpNetworkConnectionChannel::isSocketBusy() const { return (state & QHttpNetworkConnectionChannel::BusyState); @@ -694,8 +707,8 @@ void QHttpNetworkConnectionChannel::_q_disconnected() return; } - // read the available data before closing - if (isSocketWaiting() || isSocketReading()) { + // read the available data before closing (also done in _q_error for other codepaths) + if ((isSocketWaiting() || isSocketReading()) && socket->bytesAvailable()) { if (reply) { state = QHttpNetworkConnectionChannel::ReadingState; _q_receiveReply(); @@ -707,7 +720,8 @@ void QHttpNetworkConnectionChannel::_q_disconnected() state = QHttpNetworkConnectionChannel::IdleState; requeueCurrentlyPipelinedRequests(); - close(); + + pendingEncrypt = false; } @@ -789,11 +803,19 @@ void QHttpNetworkConnectionChannel::_q_error(QAbstractSocket::SocketError socket errorCode = QNetworkReply::ConnectionRefusedError; break; case QAbstractSocket::RemoteHostClosedError: - // try to reconnect/resend before sending an error. - // while "Reading" the _q_disconnected() will handle this. - if (state != QHttpNetworkConnectionChannel::IdleState && state != QHttpNetworkConnectionChannel::ReadingState) { + // This error for SSL comes twice in a row, first from SSL layer ("The TLS/SSL connection has been closed") then from TCP layer. + // Depending on timing it can also come three times in a row (first time when we try to write into a closing QSslSocket). + // The reconnectAttempts handling catches the cases where we can re-send the request. + if (!reply && state == QHttpNetworkConnectionChannel::IdleState) { + // Not actually an error, it is normal for Keep-Alive connections to close after some time if no request + // is sent on them. No need to error the other replies below. Just bail out here. + // The _q_disconnected will handle the possibly pipelined replies + return; + } else if (state != QHttpNetworkConnectionChannel::IdleState && state != QHttpNetworkConnectionChannel::ReadingState) { + // Try to reconnect/resend before sending an error. + // While "Reading" the _q_disconnected() will handle this. if (reconnectAttempts-- > 0) { - closeAndResendCurrentRequest(); + resendCurrentRequest(); return; } else { errorCode = QNetworkReply::RemoteHostClosedError; @@ -818,24 +840,15 @@ void QHttpNetworkConnectionChannel::_q_error(QAbstractSocket::SocketError socket // we can ignore the readbuffersize as the data is already // in memory and we will not receive more data on the socket. reply->setReadBufferSize(0); + reply->setDownstreamLimited(false); _q_receiveReply(); -#ifndef QT_NO_SSL - if (ssl) { - // QT_NO_OPENSSL. The QSslSocket can still have encrypted bytes in the plainsocket. - // So we need to check this if the socket is a QSslSocket. When the socket is flushed - // it will force a decrypt of the encrypted data in the plainsocket. - QSslSocket *sslSocket = static_cast(socket); - qint64 beforeFlush = sslSocket->encryptedBytesAvailable(); - while (sslSocket->encryptedBytesAvailable()) { - sslSocket->flush(); - _q_receiveReply(); - qint64 afterFlush = sslSocket->encryptedBytesAvailable(); - if (afterFlush == beforeFlush) - break; - beforeFlush = afterFlush; - } + if (!reply) { + // No more reply assigned after the previous call? Then it had been finished successfully. + requeueCurrentlyPipelinedRequests(); + state = QHttpNetworkConnectionChannel::IdleState; + QMetaObject::invokeMethod(connection, "_q_startNextRequest", Qt::QueuedConnection); + return; } -#endif } errorCode = QNetworkReply::RemoteHostClosedError; @@ -846,7 +859,7 @@ void QHttpNetworkConnectionChannel::_q_error(QAbstractSocket::SocketError socket case QAbstractSocket::SocketTimeoutError: // try to reconnect/resend before sending an error. if (state == QHttpNetworkConnectionChannel::WritingState && (reconnectAttempts-- > 0)) { - closeAndResendCurrentRequest(); + resendCurrentRequest(); return; } errorCode = QNetworkReply::TimeoutError; @@ -860,7 +873,7 @@ void QHttpNetworkConnectionChannel::_q_error(QAbstractSocket::SocketError socket case QAbstractSocket::ProxyConnectionClosedError: // try to reconnect/resend before sending an error. if (reconnectAttempts-- > 0) { - closeAndResendCurrentRequest(); + resendCurrentRequest(); return; } errorCode = QNetworkReply::ProxyConnectionClosedError; @@ -868,7 +881,7 @@ void QHttpNetworkConnectionChannel::_q_error(QAbstractSocket::SocketError socket case QAbstractSocket::ProxyConnectionTimeoutError: // try to reconnect/resend before sending an error. if (reconnectAttempts-- > 0) { - closeAndResendCurrentRequest(); + resendCurrentRequest(); return; } errorCode = QNetworkReply::ProxyTimeoutError; @@ -916,8 +929,18 @@ void QHttpNetworkConnectionChannel::_q_error(QAbstractSocket::SocketError socket // send the next request QMetaObject::invokeMethod(that, "_q_startNextRequest", Qt::QueuedConnection); - if (that) //signal emission triggered event loop - close(); + if (that) { + //signal emission triggered event loop + if (!socket) + state = QHttpNetworkConnectionChannel::IdleState; + else if (socket->state() == QAbstractSocket::UnconnectedState) + state = QHttpNetworkConnectionChannel::IdleState; + else + state = QHttpNetworkConnectionChannel::ClosingState; + + // pendingEncrypt must only be true in between connected and encrypted states + pendingEncrypt = false; + } } #ifndef QT_NO_NETWORKPROXY @@ -941,7 +964,8 @@ void QHttpNetworkConnectionChannel::_q_proxyAuthenticationRequired(const QNetwor void QHttpNetworkConnectionChannel::_q_uploadDataReadyRead() { - sendRequest(); + if (reply) + sendRequest(); } #ifndef QT_NO_SSL diff --git a/src/network/access/qhttpnetworkconnectionchannel_p.h b/src/network/access/qhttpnetworkconnectionchannel_p.h index 231fe11..a834b7d 100644 --- a/src/network/access/qhttpnetworkconnectionchannel_p.h +++ b/src/network/access/qhttpnetworkconnectionchannel_p.h @@ -169,6 +169,7 @@ public: void handleUnexpectedEOF(); void closeAndResendCurrentRequest(); + void resendCurrentRequest(); bool isSocketBusy() const; bool isSocketWriting() const; diff --git a/src/network/access/qhttpnetworkreply.cpp b/src/network/access/qhttpnetworkreply.cpp index 55863a3..8b71bd8 100644 --- a/src/network/access/qhttpnetworkreply.cpp +++ b/src/network/access/qhttpnetworkreply.cpp @@ -191,7 +191,7 @@ QByteArray QHttpNetworkReply::readAny() return QByteArray(); // we'll take the last buffer, so schedule another read from http - if (d->downstreamLimited && d->responseData.bufferCount() == 1) + if (d->downstreamLimited && d->responseData.bufferCount() == 1 && !isFinished()) d->connection->d_func()->readMoreLater(this); return d->responseData.read(); } diff --git a/src/network/access/qhttpprotocolhandler.cpp b/src/network/access/qhttpprotocolhandler.cpp index 3357948..380aaac 100644 --- a/src/network/access/qhttpprotocolhandler.cpp +++ b/src/network/access/qhttpprotocolhandler.cpp @@ -250,7 +250,6 @@ bool QHttpProtocolHandler::sendRequest() if (!m_reply) { // heh, how should that happen! qWarning() << "QAbstractProtocolHandler::sendRequest() called without QHttpNetworkReply"; - m_channel->state = QHttpNetworkConnectionChannel::IdleState; return false; } diff --git a/src/network/socket/qabstractsocket.cpp b/src/network/socket/qabstractsocket.cpp index 2666771..0e82d4a 100644 --- a/src/network/socket/qabstractsocket.cpp +++ b/src/network/socket/qabstractsocket.cpp @@ -768,6 +768,7 @@ bool QAbstractSocketPrivate::canReadNotification() void QAbstractSocketPrivate::canCloseNotification() { Q_Q(QAbstractSocket); + // Note that this method is only called on Windows. Other platforms close in the canReadNotification() #if defined (QABSTRACTSOCKET_DEBUG) qDebug("QAbstractSocketPrivate::canCloseNotification()"); @@ -777,7 +778,11 @@ void QAbstractSocketPrivate::canCloseNotification() if (isBuffered) { // Try to read to the buffer, if the read fail we can close the socket. newBytes = buffer.size(); - if (!readFromSocket()) { + qint64 oldReadBufferMaxSize = readBufferMaxSize; + readBufferMaxSize = 0; // temporarily disable max read buffer, we want to empty the OS buffer + bool hadReadFromSocket = readFromSocket(); + readBufferMaxSize = oldReadBufferMaxSize; + if (!hadReadFromSocket) { q->disconnectFromHost(); return; } diff --git a/src/network/ssl/qsslsocket.cpp b/src/network/ssl/qsslsocket.cpp index c1fab94..2b9e923 100644 --- a/src/network/ssl/qsslsocket.cpp +++ b/src/network/ssl/qsslsocket.cpp @@ -2294,6 +2294,14 @@ void QSslSocketPrivate::_q_errorSlot(QAbstractSocket::SocketError error) qCDebug(lcSsl) << "\tstate =" << q->state(); qCDebug(lcSsl) << "\terrorString =" << q->errorString(); #endif + // this moves encrypted bytes from plain socket into our buffer + if (plainSocket->bytesAvailable()) { + qint64 tmpReadBufferMaxSize = readBufferMaxSize; + readBufferMaxSize = 0; // reset temporarily so the plain sockets completely drained drained + transmit(); + readBufferMaxSize = tmpReadBufferMaxSize; + } + q->setSocketError(plainSocket->error()); q->setErrorString(plainSocket->errorString()); emit q->error(error); diff --git a/src/network/ssl/qsslsocket_openssl.cpp b/src/network/ssl/qsslsocket_openssl.cpp index ac4336a..94655fe 100644 --- a/src/network/ssl/qsslsocket_openssl.cpp +++ b/src/network/ssl/qsslsocket_openssl.cpp @@ -1419,6 +1419,13 @@ void QSslSocketBackendPrivate::disconnected() { if (plainSocket->bytesAvailable() <= 0) destroySslContext(); + else { + // Move all bytes into the plain buffer + qint64 tmpReadBufferMaxSize = readBufferMaxSize; + readBufferMaxSize = 0; // reset temporarily so the plain socket buffer is completely drained + transmit(); + readBufferMaxSize = tmpReadBufferMaxSize; + } //if there is still buffered data in the plain socket, don't destroy the ssl context yet. //it will be destroyed when the socket is deleted. } diff --git a/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp b/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp index d2edf67..138f528 100644 --- a/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp +++ b/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp @@ -1051,7 +1051,7 @@ protected: // clean up QAbstractSocket's residue: while (client->bytesToWrite() > 0) { qDebug() << "Still having" << client->bytesToWrite() << "bytes to write, doing that now"; - if (!client->waitForBytesWritten(2000)) { + if (!client->waitForBytesWritten(10000)) { qDebug() << "ERROR: FastSender:" << client->error() << "cleaning up residue"; return; } @@ -1071,7 +1071,7 @@ protected: measuredSentBytes += writeNextData(client, bytesToWrite); while (client->bytesToWrite() > 0) { - if (!client->waitForBytesWritten(2000)) { + if (!client->waitForBytesWritten(10000)) { qDebug() << "ERROR: FastSender:" << client->error() << "during blocking write"; return; } @@ -7946,7 +7946,7 @@ public slots: m_receivedData += data; if (!m_parsedHeaders && m_receivedData.contains("\r\n\r\n")) { m_parsedHeaders = true; - QTimer::singleShot(qrand()%10, this, SLOT(closeDelayed())); // simulate random network latency + QTimer::singleShot(qrand()%60, this, SLOT(closeDelayed())); // simulate random network latency // This server simulates a web server connection closing, e.g. because of Apaches MaxKeepAliveRequests or KeepAliveTimeout // In this case QNAM needs to re-send the upload data but it had a bug which then corrupts the upload // This test catches that. @@ -8052,11 +8052,12 @@ void tst_QNetworkReply::putWithServerClosingConnectionImmediately() // get the request started and the incoming socket connected QTestEventLoop::instance().enterLoop(10); + QVERIFY(!QTestEventLoop::instance().timeout()); //qDebug() << "correct=" << server.m_correctUploads << "corrupt=" << server.m_corruptUploads << "expected=" < 5); + QVERIFY(server.m_correctUploads > 2); // Because actually important is that we don't get any corruption: QCOMPARE(server.m_corruptUploads, 0); -- 1.9.1