QNAM: try to send headers and body together

For HTTP connections, QNAM defaults to opening its TCP socket in
unbuffered mode. This means that Qt will send the data written
into the socket right to the kernel, queueing only if the kernel
says it doesn't want more data for the moment being.

QNAM itself then uses separate write() calls to write the HTTP
headers and the body of the request (like POST or PUT). These 2+
writes result in headers and body being sent over different TCP
segments -- even if, in principle, a POST with a few bytes of data
(e.g. a HTML form, or a REST or SOAP request) could fit in the same
segment as the request.

Multiple writes like this interact extremely poorly with other
TCP features, e.g. delayed ACKs, Nagle's algorithm and the like.
In a typical scenario, the kernel will send a segment containing just
the headers, wait for the ACK (which may be delayed), and only then
send the body (it wasn't sent before because Nagle was blocking it).
The reply at this point is immediate (because the server can process
the request and starts replying), but the delayed ACK is typically
40-50ms, and documented up to 500ms (!). If one uses QNAM to access a
service, this introduces unacceptable latency.

These multiple writes to the OS should be avoided.

The first thing that comes into mind is to use buffered sockets.
Now, there are good reasons to keep the socket unbuffered, so we
don't want to change that. But the deal breaker is that even buffered
sockets won't help in general: for instance, on Windows, a buffered
write will immediately detect that the socket is ready for write and
flush the buffer right away (not 100% sure of why this is necessary;
basically, after populating the QTcpSocket write buffer, Qt enables
a write socket notifier on the socket -- notifier that fires
synchronously and immediately, without even returning to the event
loop, and that causes the write buffer flush).

Linux of course offers the perfect solution: corking the socket via
TCP_CORK, which tells the kernel not to send the data right away but
to buffer it up to a timeout (or when the option gets disabled
again, whichever comes first). It's explicitly designed to support
the case of sending headers followed by something like a
sendfile(2). Setting this socket option moves the problem to
the kernel and we could happily keep issuing multiple writes.

Ça va sans dire, no other OS supports that option or any other
similar option.

We have therefore to deal with this in userspace: don't write in the
socket multiple times, but try and coalesce the write of the headers
with the writing of the data. This patch implements that, by storing
the headers and sending them together with the very first chunk of
data. If the data is small enough, this sends the entire request
in one TCP segment.

Interestingly enough, QNAM has a call setting TCP_NODELAY
currently commented out because Qt doesn't combine "HTTP requests"
(whatever that means). The call comes all the way back
from pre-public history (before 2011) (!). This patch doesn't
touch it.

Fixes: QTBUG-41907
Change-Id: Id555d14e0702c9f75c3134b18277692eb3659afe
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>

2 files changed, 38 insertions, 12 deletions

diff --git a/src/network/access/qhttpprotocolhandler.cpp b/src/network/access/qhttpprotocolhandler.cpp
index d39589fb96..69afd35b57 100644
--- a/src/network/access/qhttpprotocolhandler.cpp
+++ b/src/network/access/qhttpprotocolhandler.cpp
@@ -313,12 +313,11 @@ bool QHttpProtocolHandler::sendRequest()
         if (m_channel->request.withCredentials())
             m_connection->d_func()->createAuthorization(m_socket, m_channel->request);
 #ifndef QT_NO_NETWORKPROXY
-        QByteArray header = QHttpNetworkRequestPrivate::header(m_channel->request,
+        m_header = QHttpNetworkRequestPrivate::header(m_channel->request,
             (m_connection->d_func()->networkProxy.type() != QNetworkProxy::NoProxy));
 #else
-        QByteArray header = QHttpNetworkRequestPrivate::header(m_channel->request, false);
+        m_header = QHttpNetworkRequestPrivate::header(m_channel->request, false);
 #endif
-        m_socket->write(header);
         // flushing is dangerous (QSslSocket calls transmit which might read or error)
 //        m_socket->flush();
         QNonContiguousByteDevice* uploadByteDevice = m_channel->request.uploadByteDevice();
@@ -331,6 +330,8 @@ bool QHttpProtocolHandler::sendRequest()
             m_channel->state = QHttpNetworkConnectionChannel::WritingState; // start writing data
             sendRequest(); //recurse
         } else {
+            // no data to send: just send the HTTP headers
+            m_socket->write(qExchange(m_header, {}));
             m_channel->state = QHttpNetworkConnectionChannel::WaitingState; // now wait for response
             sendRequest(); //recurse
         }
@@ -342,6 +343,10 @@ bool QHttpProtocolHandler::sendRequest()
         // write the data
         QNonContiguousByteDevice* uploadByteDevice = m_channel->request.uploadByteDevice();
         if (!uploadByteDevice || m_channel->bytesTotal == m_channel->written) {
+            // the upload device might have no data to send, but we still have to send the headers,
+            // do it now.
+            if (!m_header.isEmpty())
+                m_socket->write(qExchange(m_header, {}));
             if (uploadByteDevice)
                 emit m_reply->dataSendProgress(m_channel->written, m_channel->bytesTotal);
             m_channel->state = QHttpNetworkConnectionChannel::WaitingState; // now wait for response
@@ -349,24 +354,31 @@ bool QHttpProtocolHandler::sendRequest()
             break;
         }
 
-        // only feed the QTcpSocket buffer when there is less than 32 kB in it
+        // only feed the QTcpSocket buffer when there is less than 32 kB in it;
+        // note that the headers do not count towards these limits.
         const qint64 socketBufferFill = 32*1024;
         const qint64 socketWriteMaxSize = 16*1024;
 
-
+        // if it is really an ssl socket, check more than just bytesToWrite()
 #ifndef QT_NO_SSL
         QSslSocket *sslSocket = qobject_cast<QSslSocket*>(m_socket);
-        // if it is really an ssl socket, check more than just bytesToWrite()
-        while ((m_socket->bytesToWrite() + (sslSocket ? sslSocket->encryptedBytesToWrite() : 0))
-                <= socketBufferFill && m_channel->bytesTotal != m_channel->written)
+        const auto encryptedBytesToWrite = [sslSocket]() -> qint64
+        {
+            return sslSocket ? sslSocket->encryptedBytesToWrite() : 0;
+        };
 #else
-        while (m_socket->bytesToWrite() <= socketBufferFill
-               && m_channel->bytesTotal != m_channel->written)
+        const auto encryptedBytesToWrite = [](){ return qint64(0); };
 #endif
+
+        // throughout this loop, we want to send the data coming from uploadByteDevice.
+        // we also need to send the headers, as we try to coalesce their write with the data.
+        // we won't send more than the limits above, excluding the headers
+        while ((m_socket->bytesToWrite() + encryptedBytesToWrite()) <= socketBufferFill
+               && m_channel->bytesTotal != m_channel->written)
         {
             // get pointer to upload data
             qint64 currentReadSize = 0;
-            qint64 desiredReadSize = qMin(socketWriteMaxSize, m_channel->bytesTotal - m_channel->written);
+            const qint64 desiredReadSize = qMin(socketWriteMaxSize, m_channel->bytesTotal - m_channel->written);
             const char *readPointer = uploadByteDevice->readPointer(desiredReadSize, currentReadSize);
 
             if (currentReadSize == -1) {
@@ -384,7 +396,17 @@ bool QHttpProtocolHandler::sendRequest()
                     m_connection->d_func()->emitReplyError(m_socket, m_reply, QNetworkReply::ProtocolFailure);
                     return false;
                 }
-                qint64 currentWriteSize = m_socket->write(readPointer, currentReadSize);
+                qint64 currentWriteSize;
+                if (m_header.isEmpty()) {
+                    currentWriteSize = m_socket->write(readPointer, currentReadSize);
+                } else {
+                    // assemble header and data and send them together
+                    const qint64 headerSize = m_header.size();
+                    m_header.append(readPointer, currentReadSize);
+                    currentWriteSize = m_socket->write(qExchange(m_header, {}));
+                    if (currentWriteSize != -1)
+                        currentWriteSize -= headerSize;
+                }
                 if (currentWriteSize == -1 || currentWriteSize != currentReadSize) {
                     // socket broke down
                     m_connection->d_func()->emitReplyError(m_socket, m_reply, QNetworkReply::UnknownNetworkError);
diff --git a/src/network/access/qhttpprotocolhandler_p.h b/src/network/access/qhttpprotocolhandler_p.h
index 8e766604bb..f2da21d3b6 100644
--- a/src/network/access/qhttpprotocolhandler_p.h
+++ b/src/network/access/qhttpprotocolhandler_p.h
@@ -55,6 +55,8 @@
 #include <QtNetwork/private/qtnetworkglobal_p.h>
 #include <private/qabstractprotocolhandler_p.h>
 
+#include <QtCore/qbytearray.h>
+
 QT_REQUIRE_CONFIG(http);
 
 QT_BEGIN_NAMESPACE
@@ -67,6 +69,8 @@ private:
     virtual void _q_receiveReply() override;
     virtual void _q_readyRead() override;
     virtual bool sendRequest() override;
+
+    QByteArray m_header;
 };
 
 QT_END_NAMESPACE