QNetworkAccessManager: don't resend non-idempotent requests

Requests that may lead to a different state when performed multiple
times (non-idempotent) should not be automatically re-transmitted if an
error occurs after we have written the full request.

We assume all custom methods are potentially non-idempotent.

[ChangeLog][QtNetwork][QNetworkAccessManager][Behavior Change]
Non-idempotent requests are no longer incorrectly re-sent if the
connection breaks down while reading the response.

Fixes: QTBUG-134694
Pick-to: 6.9 6.8 6.5
Change-Id: Ie8ba7828ce9375359c2326f06426fe1a1e568fef
Reviewed-by: Mate Barany <mate.barany@qt.io>

src/network/access/qhttpnetworkconnectionchannel.cpp

@@ -271,7 +271,7 @@ void QHttpNetworkConnectionChannel::_q_readyRead()
 void QHttpNetworkConnectionChannel::handleUnexpectedEOF()
 {
     Q_ASSERT(reply);
-    if (reconnectAttempts <= 0) {
+    if (reconnectAttempts <= 0 || !request.methodIsIdempotent()) {
         // too many errors reading/receiving/parsing the status, close the socket and emit error
         requeueCurrentlyPipelinedRequests();
         close();

src/network/access/qhttpnetworkrequest.cpp

@@ -394,5 +394,13 @@ void QHttpNetworkRequest::setFullLocalServerName(const QString &fullServerName)
     d->fullLocalServerName = fullServerName;
 }
 
+bool QHttpNetworkRequest::methodIsIdempotent() const
+{
+    using Op = Operation;
+    constexpr auto knownSafe = std::array{ Op::Get, Op::Head, Op::Put, Op::Trace, Op::Options };
+    return std::any_of(knownSafe.begin(), knownSafe.end(),
+                       [currentOp = d->operation](auto op) { return op == currentOp; });
+}
+
 QT_END_NAMESPACE
 

src/network/access/qhttpnetworkrequest_p.h

@@ -120,6 +120,8 @@ public:
     QString fullLocalServerName() const;
     void setFullLocalServerName(const QString &fullServerName);
 
+    bool methodIsIdempotent() const;
+
 private:
     QSharedDataPointer<QHttpNetworkRequestPrivate> d;
     friend class QHttpNetworkRequestPrivate;

tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp

@@ -600,6 +600,9 @@ private Q_SLOTS:
 
     void abortAndError();
 
+    void resendRequest_data();
+    void resendRequest();
+
     // NOTE: This test must be last!
     void parentingRepliesToTheApp();
 private:
@@ -678,6 +681,7 @@ public:
     QByteArray receivedData;
     QSemaphore ready;
     bool doClose;
+    bool earlyClose = false; // close connection after request has been received
     bool doSsl;
     bool ipv6;
     bool multiple;
@@ -805,6 +809,9 @@ private slots:
         qDebug() << "slotError" << err << currentClient->errorString();
     }
 
+signals:
+    void requestReceived() const;
+
 public slots:
 
     void readyReadSlot()
@@ -830,10 +837,16 @@ public slots:
             if (contentRead < contentLength)
                 return;
 
+            emit requestReceived();
+
             // multiple requests incoming. remove the bytes of the current one
             if (multiple)
                 receivedData.remove(0, endOfHeader);
 
+            if (earlyClose) {
+                client->disconnectFromHost();
+                return;
+            }
             reply();
         }
     }
@@ -10740,6 +10753,56 @@ Hello World!)"_ba;
     QCOMPARE(reply->error(), QNetworkReply::OperationCanceledError);
 }
 
+void tst_QNetworkReply::resendRequest_data(){
+    QTest::addColumn<QString>("method");
+    QTest::addColumn<bool>("shouldResend");
+
+    for (auto &method : { "get", "head", "put" })
+        QTest::addRow("%s", method) << method << true;
+    QTest::addRow("post") << "post" << false;
+    QTest::addRow("mycustom") << "mycustom" << false;
+
+}
+
+void tst_QNetworkReply::resendRequest()
+{
+    QFETCH(const QString, method);
+    QFETCH(const bool, shouldResend);
+
+    MiniHttpServer server("");
+    server.earlyClose = true;
+
+    QSignalSpy requestReceived(&server, &MiniHttpServer::requestReceived);
+
+    QUrl url("http://127.0.0.1");
+    url.setPort(server.serverPort());
+    const QByteArray data(4096, 'a');
+    QNetworkReplyPtr reply([&]() {
+        QNetworkRequest req(url);
+        if (method == "get")
+            return manager.get(req, data);
+        else if (method == "head")
+            return manager.head(req);
+        else if (method == "put")
+            return manager.put(req, data);
+        else
+            return manager.sendCustomRequest(req, method.toUtf8(), data);
+    }());
+
+    // We send one request and will get no response from the server:
+    QVERIFY(requestReceived.wait());
+    requestReceived.clear();
+    // Then, for idempotent requests, we send the request again. For
+    // non-idempotent requests we error out and don't try to resend.
+    QCOMPARE(requestReceived.wait(2s), shouldResend);
+    if (!shouldResend) {
+        QCOMPARE(reply->error(), QNetworkReply::RemoteHostClosedError);
+    } else {
+        // No error yet, still can resend another
+        QCOMPARE(reply->error(), QNetworkReply::NoError);
+    }
+}
+
 // NOTE: This test must be last testcase in tst_qnetworkreply!
 void tst_QNetworkReply::parentingRepliesToTheApp()
 {