From 9917eb2ec69c2d5cc1db408660df43af34fe63fb Mon Sep 17 00:00:00 2001 From: Timur Pocheptsov Date: Thu, 5 Apr 2018 15:51:36 +0200 Subject: HTTP/2 - reset uploadByteDevice if necessary 1. If a request was redirected or some error was encountered, we try to reset the uploading byte-device. 2. Disconnecting from the byte-device is not enough, since we have a queued connection, _q_uploadDataReadyRead() gets called even if byte-device was deleted and thus sender() can return null - we have to check this condition. 3. Update auto-test with a case where our server immediately replies with a redirect status code. Task-number: QTBUG-67469 Task-number: QTBUG-66913 Change-Id: I9b364cf3dee1717940ddbe50cba37c3398cc9c95 Reviewed-by: Edward Welbourne --- src/network/access/qhttp2protocolhandler.cpp | 14 ++++++++ tests/auto/network/access/http2/http2srv.cpp | 28 ++++++++++++++-- tests/auto/network/access/http2/http2srv.h | 5 +++ tests/auto/network/access/http2/tst_http2.cpp | 47 +++++++++++++++++++++++++++ 4 files changed, 92 insertions(+), 2 deletions(-) diff --git a/src/network/access/qhttp2protocolhandler.cpp b/src/network/access/qhttp2protocolhandler.cpp index 5420e713b5..22541e83ba 100644 --- a/src/network/access/qhttp2protocolhandler.cpp +++ b/src/network/access/qhttp2protocolhandler.cpp @@ -212,6 +212,9 @@ QHttp2ProtocolHandler::QHttp2ProtocolHandler(QHttpNetworkConnectionChannel *chan void QHttp2ProtocolHandler::_q_uploadDataReadyRead() { + if (!sender()) // QueuedConnection, firing after sender (byte device) was deleted. + return; + auto data = qobject_cast(sender()); Q_ASSERT(data); const qint32 streamID = data->property("HTTP2StreamID").toInt(); @@ -1110,6 +1113,17 @@ void QHttp2ProtocolHandler::updateStream(Stream &stream, const HPack::HttpHeader if (QHttpNetworkReply::isHttpRedirect(statusCode) && redirectUrl.isValid()) httpReply->setRedirectUrl(redirectUrl); + if (QHttpNetworkReply::isHttpRedirect(statusCode) + || statusCode == 401 || statusCode == 407) { + // These are the status codes that can trigger uploadByteDevice->reset() + // in QHttpNetworkConnectionChannel::handleStatus. Alas, we have no + // single request/reply, we multiplex several requests and thus we never + // simply call 'handleStatus'. If we have byte-device - we try to reset + // it here, we don't (and can't) handle any error during reset operation. + if (stream.data()) + stream.data()->reset(); + } + if (connectionType == Qt::DirectConnection) emit httpReply->headerChanged(); else diff --git a/tests/auto/network/access/http2/http2srv.cpp b/tests/auto/network/access/http2/http2srv.cpp index 69e480b164..b0bae13bad 100644 --- a/tests/auto/network/access/http2/http2srv.cpp +++ b/tests/auto/network/access/http2/http2srv.cpp @@ -123,6 +123,12 @@ void Http2Server::emulateGOAWAY(int timeout) goawayTimeout = timeout; } +void Http2Server::redirectOpenStream(quint16 port) +{ + redirectWhileReading = true; + targetPort = port; +} + void Http2Server::startServer() { #ifdef QT_NO_SSL @@ -775,7 +781,19 @@ void Http2Server::sendResponse(quint32 streamID, bool emptyBody) if (emptyBody) writer.addFlag(FrameFlag::END_STREAM); - HttpHeader header = {{":status", "200"}}; + HttpHeader header; + if (redirectWhileReading) { + qDebug("server received HEADERS frame (followed by DATA frames), redirecting ..."); + Q_ASSERT(targetPort); + header.push_back({":status", "308"}); + const QString url("%1://localhost:%2/"); + header.push_back({"location", url.arg(clearTextHTTP2 ? QStringLiteral("http") : QStringLiteral("https"), + QString::number(targetPort)).toLatin1()}); + + } else { + header.push_back({":status", "200"}); + } + if (!emptyBody) { header.push_back(HPack::HeaderField("content-length", QString("%1").arg(responseBody.size()).toLatin1())); @@ -871,7 +889,13 @@ void Http2Server::processRequest() activeRequests[streamID] = decoder.decodedHeader(); if (headersFrame.flags().testFlag(FrameFlag::END_STREAM)) emit receivedRequest(streamID); - // else - we're waiting for incoming DATA frames ... + + if (redirectWhileReading) { + sendResponse(streamID, true); + // Don't try to read any DATA frames ... + socket->disconnect(); + } // else - we're waiting for incoming DATA frames ... + continuedRequest.clear(); } diff --git a/tests/auto/network/access/http2/http2srv.h b/tests/auto/network/access/http2/http2srv.h index 76c5a0ee36..14b41cc67d 100644 --- a/tests/auto/network/access/http2/http2srv.h +++ b/tests/auto/network/access/http2/http2srv.h @@ -74,6 +74,7 @@ public: void enablePushPromise(bool enabled, const QByteArray &path = QByteArray()); void setResponseBody(const QByteArray &body); void emulateGOAWAY(int timeout); + void redirectOpenStream(quint16 targetPort); // Invokables, since we can call them from the main thread, // but server (can) work on its own thread. @@ -186,6 +187,10 @@ private: // We need it for PUSH_PROMISE, with the correct port number appended, // when replying to essentially 1.1 request. QByteArray authority; + // Redirect, with status code 308, as soon as we've seen headers, while client + // may still be sending DATA frames. See tst_Http2::earlyResponse(). + bool redirectWhileReading = false; + quint16 targetPort = 0; protected slots: void ignoreErrorSlot(); }; diff --git a/tests/auto/network/access/http2/tst_http2.cpp b/tests/auto/network/access/http2/tst_http2.cpp index 51e1849512..ecf4c5814a 100644 --- a/tests/auto/network/access/http2/tst_http2.cpp +++ b/tests/auto/network/access/http2/tst_http2.cpp @@ -74,6 +74,7 @@ private slots: void pushPromise(); void goaway_data(); void goaway(); + void earlyResponse(); protected slots: // Slots to listen to our in-process server: @@ -439,6 +440,47 @@ void tst_Http2::goaway() QVERIFY(!serverGotSettingsACK); } +void tst_Http2::earlyResponse() +{ + // In this test we'd like to verify client side can handle HEADERS frame while + // its stream is in 'open' state. To achieve this, we send a POST request + // with some payload, so that the client is first sending HEADERS and then + // DATA frames without END_STREAM flag set yet (thus the stream is in Stream::open + // state). Upon receiving the client's HEADERS frame our server ('redirector') + // immediately (without trying to read any DATA frames) responds with status + // code 308. The client should properly handle this. + + clearHTTP2State(); + + serverPort = 0; + nRequests = 1; + + ServerPtr targetServer(newServer(defaultServerSettings)); + + QMetaObject::invokeMethod(targetServer.data(), "startServer", Qt::QueuedConnection); + runEventLoop(); + + QVERIFY(serverPort != 0); + + const quint16 targetPort = serverPort; + serverPort = 0; + + ServerPtr redirector(newServer(defaultServerSettings)); + redirector->redirectOpenStream(targetPort); + + QMetaObject::invokeMethod(redirector.data(), "startServer", Qt::QueuedConnection); + runEventLoop(); + + QVERIFY(serverPort); + sendRequest(1, QNetworkRequest::NormalPriority, {10000000, Qt::Uninitialized}); + + runEventLoop(); + + QVERIFY(nRequests == 0); + QVERIFY(prefaceOK); + QVERIFY(serverGotSettingsACK); +} + void tst_Http2::serverStarted(quint16 port) { serverPort = port; @@ -500,6 +542,7 @@ void tst_Http2::sendRequest(int streamNumber, QNetworkRequest request(url); request.setAttribute(QNetworkRequest::HTTP2AllowedAttribute, QVariant(true)); + request.setAttribute(QNetworkRequest::FollowRedirectsAttribute, QVariant(true)); request.setHeader(QNetworkRequest::ContentTypeHeader, QVariant("text/plain")); request.setPriority(priority); @@ -592,6 +635,10 @@ void tst_Http2::replyFinished() const QVariant spdyUsed(reply->attribute(QNetworkRequest::SpdyWasUsedAttribute)); QVERIFY(spdyUsed.isValid()); QVERIFY(!spdyUsed.toBool()); + const QVariant code(reply->attribute(QNetworkRequest::HttpStatusCodeAttribute)); + QVERIFY(code.isValid()); + QVERIFY(code.canConvert()); + QCOMPARE(code.value(), 200); } --nRequests; -- cgit v1.2.3