From dbfa374a8610948f6e03ea1b3f3bc2905ce05be0 Mon Sep 17 00:00:00 2001 From: Timur Pocheptsov Date: Mon, 2 Sep 2019 10:43:29 +0200 Subject: QHttpNetworkConnectionChannel - avoid re-connecting on 'disconnected' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The _q_error slot has a special case for RemoteHostClosedError, where the current channel's state is 'idle' and no request/reply is in progress. The comment states that: "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." _q_disconnected, indeed, takes care about pipelined replies ... calling 'ensureConnected' even if we have 0 replies in pipeline, which makes zero sense to me and results in QNAM endlessly trying to re-connect to the server. Fixes: QTBUG-77852 Change-Id: I6dcb43b36a6d432bc940246a08f65e1ee903fd24 Reviewed-by: Volker Hilsheimer Reviewed-by: Mårten Nordheim Reviewed-by: Timur Pocheptsov --- src/network/access/qhttpnetworkconnectionchannel.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'src/network/access') diff --git a/src/network/access/qhttpnetworkconnectionchannel.cpp b/src/network/access/qhttpnetworkconnectionchannel.cpp index 38adca2633..e7af7b648e 100644 --- a/src/network/access/qhttpnetworkconnectionchannel.cpp +++ b/src/network/access/qhttpnetworkconnectionchannel.cpp @@ -849,8 +849,11 @@ void QHttpNetworkConnectionChannel::_q_disconnected() QMetaObject::invokeMethod(connection, "_q_startNextRequest", Qt::QueuedConnection); } state = QHttpNetworkConnectionChannel::IdleState; - - requeueCurrentlyPipelinedRequests(); + if (alreadyPipelinedRequests.length()) { + // If nothing was in a pipeline, no need in calling + // _q_startNextRequest (which it does): + requeueCurrentlyPipelinedRequests(); + } pendingEncrypt = false; } -- cgit v1.2.3 From d1a22bd29856807805fa56608d2953c072df0cf1 Mon Sep 17 00:00:00 2001 From: Timur Pocheptsov Date: Wed, 4 Sep 2019 15:00:31 +0200 Subject: A follow-up to a recent fix in QHttpNetworkConnectionChannel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While working with HTTP/2, we are not re-sending failed requests. In case we receive a GOAWAY frame, we properly handle it by processing some active streams if possible, and aborting streams that will not proceed further with ContentResendError. But it's possible that some server failed to send us GOAWAY (for example, it died) or closed the connection not finishing the streams that were still active and valid (ID <= value from GOAWAY frame). Now that we will not re-connect, there is no reason to be quiet about us not progressing - emit RemoteHostClosedError on any remaining active stream/request we cannot process further. Fixes: QTBUG-77852 Change-Id: I4cd68a1c8c103b1fbe36c20a1cc406ab2e20dd12 Reviewed-by: Mårten Nordheim Reviewed-by: Timur Pocheptsov (cherry picked from commit 543769666f18f79bd6ebd6119a39834aafc2b0df) --- src/network/access/qhttp2protocolhandler.cpp | 25 ++++++++++++++++++++++ src/network/access/qhttp2protocolhandler_p.h | 2 ++ .../access/qhttpnetworkconnectionchannel.cpp | 13 ++++++++++- 3 files changed, 39 insertions(+), 1 deletion(-) (limited to 'src/network/access') diff --git a/src/network/access/qhttp2protocolhandler.cpp b/src/network/access/qhttp2protocolhandler.cpp index 5d7fc7506e..6609aa0594 100644 --- a/src/network/access/qhttp2protocolhandler.cpp +++ b/src/network/access/qhttp2protocolhandler.cpp @@ -55,6 +55,8 @@ #include #endif +#include + #include #include @@ -211,6 +213,29 @@ QHttp2ProtocolHandler::QHttp2ProtocolHandler(QHttpNetworkConnectionChannel *chan } } +void QHttp2ProtocolHandler::handleConnectionClosure() +{ + // The channel has just received RemoteHostClosedError and since it will + // not try (for HTTP/2) to re-connect, it's time to finish all replies + // with error. + + // Maybe we still have some data to read and can successfully finish + // a stream/request? + _q_receiveReply(); + + // Finish all still active streams. If we previously had GOAWAY frame, + // we probably already closed some (or all) streams with ContentReSend + // error, but for those still active, not having any data to finish, + // we now report RemoteHostClosedError. + const auto errorString = QCoreApplication::translate("QHttp", "Connection closed"); + for (auto it = activeStreams.begin(), eIt = activeStreams.end(); it != eIt; ++it) + finishStreamWithError(it.value(), QNetworkReply::RemoteHostClosedError, errorString); + + // Make sure we'll never try to read anything later: + activeStreams.clear(); + goingAway = true; +} + void QHttp2ProtocolHandler::_q_uploadDataReadyRead() { if (!sender()) // QueuedConnection, firing after sender (byte device) was deleted. diff --git a/src/network/access/qhttp2protocolhandler_p.h b/src/network/access/qhttp2protocolhandler_p.h index 9165808302..d91853f613 100644 --- a/src/network/access/qhttp2protocolhandler_p.h +++ b/src/network/access/qhttp2protocolhandler_p.h @@ -90,6 +90,8 @@ public: QHttp2ProtocolHandler &operator = (const QHttp2ProtocolHandler &rhs) = delete; QHttp2ProtocolHandler &operator = (QHttp2ProtocolHandler &&rhs) = delete; + Q_INVOKABLE void handleConnectionClosure(); + private slots: void _q_uploadDataReadyRead(); void _q_replyDestroyed(QObject* reply); diff --git a/src/network/access/qhttpnetworkconnectionchannel.cpp b/src/network/access/qhttpnetworkconnectionchannel.cpp index e7af7b648e..1fac24ab49 100644 --- a/src/network/access/qhttpnetworkconnectionchannel.cpp +++ b/src/network/access/qhttpnetworkconnectionchannel.cpp @@ -967,7 +967,18 @@ void QHttpNetworkConnectionChannel::_q_error(QAbstractSocket::SocketError socket 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 + // The _q_disconnected will handle the possibly pipelined replies. HTTP/2 is special for now, + // we do not resend, but must report errors if any request is in progress (note, while + // not in its sendRequest(), protocol handler switches the channel to IdleState, thus + // this check is under this condition in 'if'): + if (protocolHandler.data()) { + if (connection->connectionType() == QHttpNetworkConnection::ConnectionTypeHTTP2Direct + || connection->connectionType() == QHttpNetworkConnection::ConnectionTypeHTTP2) { + auto h2Handler = static_cast(protocolHandler.data()); + h2Handler->handleConnectionClosure(); + protocolHandler.reset(); + } + } return; } else if (state != QHttpNetworkConnectionChannel::IdleState && state != QHttpNetworkConnectionChannel::ReadingState) { // Try to reconnect/resend before sending an error. -- cgit v1.2.3