Compare commits
No commits in common. 'c9' and 'i9c-beta' have entirely different histories.
@ -1,232 +0,0 @@
|
|||||||
From b1e75376cc3adfc7da5502a277dfe9711f3e0536 Mon Sep 17 00:00:00 2001
|
|
||||||
From: Mårten Nordheim <marten.nordheim@qt.io>
|
|
||||||
Date: Tue, 25 Jun 2024 17:09:35 +0200
|
|
||||||
Subject: [PATCH] HTTP2: Delay any communication until encrypted() can be responded to
|
|
||||||
|
|
||||||
We have the encrypted() signal that lets users do extra checks on the
|
|
||||||
established connection. It is emitted as BlockingQueued, so the HTTP
|
|
||||||
thread stalls until it is done emitting. Users can potentially call
|
|
||||||
abort() on the QNetworkReply at that point, which is passed as a Queued
|
|
||||||
call back to the HTTP thread. That means that any currently queued
|
|
||||||
signal emission will be processed before the abort() call is processed.
|
|
||||||
|
|
||||||
In the case of HTTP2 it is a little special since it is multiplexed and
|
|
||||||
the code is built to start requests as they are available. This means
|
|
||||||
that, while the code worked fine for HTTP1, since one connection only
|
|
||||||
has one request, it is not working for HTTP2, since we try to send more
|
|
||||||
requests in-between the encrypted() signal and the abort() call.
|
|
||||||
|
|
||||||
This patch changes the code to delay any communication until the
|
|
||||||
encrypted() signal has been emitted and processed, for HTTP2 only.
|
|
||||||
It's done by adding a few booleans, both to know that we have to return
|
|
||||||
early and so we can keep track of what events arose and what we need to
|
|
||||||
resume once enough time has passed that any abort() call must have been
|
|
||||||
processed.
|
|
||||||
|
|
||||||
Fixes: QTBUG-126610
|
|
||||||
Pick-to: 6.8 6.7 6.5 6.2 5.15 5.12
|
|
||||||
Change-Id: Ic25a600c278203256e35f541026f34a8783235ae
|
|
||||||
Reviewed-by: Marc Mutz <marc.mutz@qt.io>
|
|
||||||
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>
|
|
||||||
---
|
|
||||||
|
|
||||||
diff --git a/src/network/access/qhttp2protocolhandler.cpp b/src/network/access/qhttp2protocolhandler.cpp
|
|
||||||
index 39dd4608..bc1dac68 100644
|
|
||||||
--- a/src/network/access/qhttp2protocolhandler.cpp
|
|
||||||
+++ b/src/network/access/qhttp2protocolhandler.cpp
|
|
||||||
@@ -371,12 +371,12 @@ bool QHttp2ProtocolHandler::sendRequest()
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
- if (!prefaceSent && !sendClientPreface())
|
|
||||||
- return false;
|
|
||||||
-
|
|
||||||
if (!requests.size())
|
|
||||||
return true;
|
|
||||||
|
|
||||||
+ if (!prefaceSent && !sendClientPreface())
|
|
||||||
+ return false;
|
|
||||||
+
|
|
||||||
m_channel->state = QHttpNetworkConnectionChannel::WritingState;
|
|
||||||
// Check what was promised/pushed, maybe we do not have to send a request
|
|
||||||
// and have a response already?
|
|
||||||
diff --git a/src/network/access/qhttpnetworkconnectionchannel.cpp b/src/network/access/qhttpnetworkconnectionchannel.cpp
|
|
||||||
index 7620ca16..e15b0810 100644
|
|
||||||
--- a/src/network/access/qhttpnetworkconnectionchannel.cpp
|
|
||||||
+++ b/src/network/access/qhttpnetworkconnectionchannel.cpp
|
|
||||||
@@ -255,6 +255,10 @@ void QHttpNetworkConnectionChannel::abort()
|
|
||||||
bool QHttpNetworkConnectionChannel::sendRequest()
|
|
||||||
{
|
|
||||||
Q_ASSERT(!protocolHandler.isNull());
|
|
||||||
+ if (waitingForPotentialAbort) {
|
|
||||||
+ needInvokeSendRequest = true;
|
|
||||||
+ return false; // this return value is unused
|
|
||||||
+ }
|
|
||||||
return protocolHandler->sendRequest();
|
|
||||||
}
|
|
||||||
|
|
||||||
@@ -267,21 +271,28 @@ bool QHttpNetworkConnectionChannel::sendRequest()
|
|
||||||
void QHttpNetworkConnectionChannel::sendRequestDelayed()
|
|
||||||
{
|
|
||||||
QMetaObject::invokeMethod(this, [this] {
|
|
||||||
- Q_ASSERT(!protocolHandler.isNull());
|
|
||||||
if (reply)
|
|
||||||
- protocolHandler->sendRequest();
|
|
||||||
+ sendRequest();
|
|
||||||
}, Qt::ConnectionType::QueuedConnection);
|
|
||||||
}
|
|
||||||
|
|
||||||
void QHttpNetworkConnectionChannel::_q_receiveReply()
|
|
||||||
{
|
|
||||||
Q_ASSERT(!protocolHandler.isNull());
|
|
||||||
+ if (waitingForPotentialAbort) {
|
|
||||||
+ needInvokeReceiveReply = true;
|
|
||||||
+ return;
|
|
||||||
+ }
|
|
||||||
protocolHandler->_q_receiveReply();
|
|
||||||
}
|
|
||||||
|
|
||||||
void QHttpNetworkConnectionChannel::_q_readyRead()
|
|
||||||
{
|
|
||||||
Q_ASSERT(!protocolHandler.isNull());
|
|
||||||
+ if (waitingForPotentialAbort) {
|
|
||||||
+ needInvokeReadyRead = true;
|
|
||||||
+ return;
|
|
||||||
+ }
|
|
||||||
protocolHandler->_q_readyRead();
|
|
||||||
}
|
|
||||||
|
|
||||||
@@ -1289,7 +1300,18 @@ void QHttpNetworkConnectionChannel::_q_encrypted()
|
|
||||||
// Similar to HTTP/1.1 counterpart below:
|
|
||||||
const auto &pairs = spdyRequestsToSend.values(); // (request, reply)
|
|
||||||
const auto &pair = pairs.first();
|
|
||||||
+ waitingForPotentialAbort = true;
|
|
||||||
emit pair.second->encrypted();
|
|
||||||
+
|
|
||||||
+ // We don't send or handle any received data until any effects from
|
|
||||||
+ // emitting encrypted() have been processed. This is necessary
|
|
||||||
+ // because the user may have called abort(). We may also abort the
|
|
||||||
+ // whole connection if the request has been aborted and there is
|
|
||||||
+ // no more requests to send.
|
|
||||||
+ QMetaObject::invokeMethod(this,
|
|
||||||
+ &QHttpNetworkConnectionChannel::checkAndResumeCommunication,
|
|
||||||
+ Qt::QueuedConnection);
|
|
||||||
+
|
|
||||||
// In case our peer has sent us its settings (window size, max concurrent streams etc.)
|
|
||||||
// let's give _q_receiveReply a chance to read them first ('invokeMethod', QueuedConnection).
|
|
||||||
QMetaObject::invokeMethod(connection, "_q_startNextRequest", Qt::QueuedConnection);
|
|
||||||
@@ -1307,6 +1329,28 @@ void QHttpNetworkConnectionChannel::_q_encrypted()
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
+void QHttpNetworkConnectionChannel::checkAndResumeCommunication()
|
|
||||||
+{
|
|
||||||
+ Q_ASSERT(connection->connectionType() == QHttpNetworkConnection::ConnectionTypeSPDY
|
|
||||||
+ || connection->connectionType() == QHttpNetworkConnection::ConnectionTypeHTTP2
|
|
||||||
+ || connection->connectionType() == QHttpNetworkConnection::ConnectionTypeHTTP2Direct);
|
|
||||||
+
|
|
||||||
+ // Because HTTP/2 requires that we send a SETTINGS frame as the first thing we do, and respond
|
|
||||||
+ // to a SETTINGS frame with an ACK, we need to delay any handling until we can ensure that any
|
|
||||||
+ // effects from emitting encrypted() have been processed.
|
|
||||||
+ // This function is called after encrypted() was emitted, so check for changes.
|
|
||||||
+
|
|
||||||
+ if (!reply && spdyRequestsToSend.isEmpty())
|
|
||||||
+ abort();
|
|
||||||
+ waitingForPotentialAbort = false;
|
|
||||||
+ if (needInvokeReadyRead)
|
|
||||||
+ _q_readyRead();
|
|
||||||
+ if (needInvokeReceiveReply)
|
|
||||||
+ _q_receiveReply();
|
|
||||||
+ if (needInvokeSendRequest)
|
|
||||||
+ sendRequest();
|
|
||||||
+}
|
|
||||||
+
|
|
||||||
void QHttpNetworkConnectionChannel::requeueSpdyRequests()
|
|
||||||
{
|
|
||||||
QList<HttpMessagePair> spdyPairs = spdyRequestsToSend.values();
|
|
||||||
diff --git a/src/network/access/qhttpnetworkconnectionchannel_p.h b/src/network/access/qhttpnetworkconnectionchannel_p.h
|
|
||||||
index d8ac3979..eac44464 100644
|
|
||||||
--- a/src/network/access/qhttpnetworkconnectionchannel_p.h
|
|
||||||
+++ b/src/network/access/qhttpnetworkconnectionchannel_p.h
|
|
||||||
@@ -107,6 +107,10 @@ public:
|
|
||||||
QAbstractSocket *socket;
|
|
||||||
bool ssl;
|
|
||||||
bool isInitialized;
|
|
||||||
+ bool waitingForPotentialAbort = false;
|
|
||||||
+ bool needInvokeReceiveReply = false;
|
|
||||||
+ bool needInvokeReadyRead = false;
|
|
||||||
+ bool needInvokeSendRequest = false;
|
|
||||||
ChannelState state;
|
|
||||||
QHttpNetworkRequest request; // current request, only used for HTTP
|
|
||||||
QHttpNetworkReply *reply; // current reply for this request, only used for HTTP
|
|
||||||
@@ -187,6 +191,8 @@ public:
|
|
||||||
void closeAndResendCurrentRequest();
|
|
||||||
void resendCurrentRequest();
|
|
||||||
|
|
||||||
+ void checkAndResumeCommunication();
|
|
||||||
+
|
|
||||||
bool isSocketBusy() const;
|
|
||||||
bool isSocketWriting() const;
|
|
||||||
bool isSocketWaiting() const;
|
|
||||||
diff --git a/tests/auto/network/access/http2/tst_http2.cpp b/tests/auto/network/access/http2/tst_http2.cpp
|
|
||||||
index bf8c7799..8ed95b99 100644
|
|
||||||
--- a/tests/auto/network/access/http2/tst_http2.cpp
|
|
||||||
+++ b/tests/auto/network/access/http2/tst_http2.cpp
|
|
||||||
@@ -118,6 +118,8 @@ private slots:
|
|
||||||
void redirect_data();
|
|
||||||
void redirect();
|
|
||||||
|
|
||||||
+ void abortOnEncrypted();
|
|
||||||
+
|
|
||||||
protected slots:
|
|
||||||
// Slots to listen to our in-process server:
|
|
||||||
void serverStarted(quint16 port);
|
|
||||||
@@ -1024,6 +1026,48 @@ void tst_Http2::redirect()
|
|
||||||
QTRY_VERIFY(serverGotSettingsACK);
|
|
||||||
}
|
|
||||||
|
|
||||||
+void tst_Http2::abortOnEncrypted()
|
|
||||||
+{
|
|
||||||
+#if !QT_CONFIG(ssl)
|
|
||||||
+ QSKIP("TLS support is needed for this test");
|
|
||||||
+#else
|
|
||||||
+ clearHTTP2State();
|
|
||||||
+ serverPort = 0;
|
|
||||||
+
|
|
||||||
+ ServerPtr targetServer(newServer(defaultServerSettings, H2Type::h2Direct));
|
|
||||||
+
|
|
||||||
+ QMetaObject::invokeMethod(targetServer.data(), "startServer", Qt::QueuedConnection);
|
|
||||||
+ runEventLoop();
|
|
||||||
+
|
|
||||||
+ nRequests = 1;
|
|
||||||
+ nSentRequests = 0;
|
|
||||||
+
|
|
||||||
+ const auto url = requestUrl(H2Type::h2Direct);
|
|
||||||
+ QNetworkRequest request(url);
|
|
||||||
+ request.setAttribute(QNetworkRequest::Http2DirectAttribute, true);
|
|
||||||
+
|
|
||||||
+ std::unique_ptr<QNetworkReply> reply{manager->get(request)};
|
|
||||||
+ reply->ignoreSslErrors();
|
|
||||||
+ connect(reply.get(), &QNetworkReply::encrypted, reply.get(), [reply = reply.get()](){
|
|
||||||
+ reply->abort();
|
|
||||||
+ });
|
|
||||||
+ connect(reply.get(), &QNetworkReply::errorOccurred, this, &tst_Http2::replyFinishedWithError);
|
|
||||||
+
|
|
||||||
+ runEventLoop();
|
|
||||||
+ STOP_ON_FAILURE
|
|
||||||
+
|
|
||||||
+ QCOMPARE(nRequests, 0);
|
|
||||||
+ QCOMPARE(reply->error(), QNetworkReply::OperationCanceledError);
|
|
||||||
+
|
|
||||||
+ const bool res = QTest::qWaitFor(
|
|
||||||
+ [this, server = targetServer.get()]() {
|
|
||||||
+ return serverGotSettingsACK || prefaceOK || nSentRequests > 0;
|
|
||||||
+ },
|
|
||||||
+ 500);
|
|
||||||
+ QVERIFY(!res);
|
|
||||||
+#endif // QT_CONFIG(ssl)
|
|
||||||
+}
|
|
||||||
+
|
|
||||||
void tst_Http2::serverStarted(quint16 port)
|
|
||||||
{
|
|
||||||
serverPort = port;
|
|
||||||
Loading…
Reference in new issue