From f46e1f6775b5e052ebe12f9db2eb211d1622274b Mon Sep 17 00:00:00 2001
From: Christian Mollekopf <chrigi_1@fastmail.fm>
Date: Thu, 2 Apr 2015 17:22:24 +0200
Subject: ResourceAccess: copy the buffer before capturing it in the lambda.

It's lifetime is limited to the end of the function, so we have to copy it before.
I switched to QByteArray because it simplifies the code and shouldn't really
cost anything, additionally the implicit sharing makes copies cheap.

This patch incurs the cost of always copying the buffer instead of writing
straight to the socket, but we probably anyways want to keep a copy around,
and if it would indeed be a performance issues (I doubt it), we could still optimize
that copy away.
---
 common/resourceaccess.cpp | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/common/resourceaccess.cpp b/common/resourceaccess.cpp
index 22d7d97..7320e50 100644
--- a/common/resourceaccess.cpp
+++ b/common/resourceaccess.cpp
@@ -41,23 +41,14 @@ class QueuedCommand
 public:
     QueuedCommand(int commandId, const std::function<void(int, const QString &)> &callback)
         : commandId(commandId),
-          bufferSize(0),
-          buffer(0),
           callback(callback)
     {}
 
-    QueuedCommand(int commandId, flatbuffers::FlatBufferBuilder &fbb, const std::function<void(int, const QString &)> &callback)
+    QueuedCommand(int commandId, const QByteArray &b, const std::function<void(int, const QString &)> &callback)
         : commandId(commandId),
-          bufferSize(fbb.GetSize()),
-          buffer(new char[bufferSize]),
+          buffer(b),
           callback(callback)
     {
-        memcpy(buffer, fbb.GetBufferPointer(), bufferSize);
-    }
-
-    ~QueuedCommand()
-    {
-        delete[] buffer;
     }
 
 private:
@@ -66,8 +57,7 @@ private:
 
 public:
     const int commandId;
-    const uint bufferSize;
-    char *buffer;
+    QByteArray buffer;
     std::function<void(int, const QString &)> callback;
 };
 
@@ -159,7 +149,9 @@ Async::Job<void> ResourceAccess::sendCommand(int commandId)
 
 Async::Job<void>  ResourceAccess::sendCommand(int commandId, flatbuffers::FlatBufferBuilder &fbb)
 {
-    return Async::start<void>([commandId, &fbb, this](Async::Future<void> &f) {
+    //The flatbuffer is transient, but we want to store it until the job is executed
+    QByteArray buffer(reinterpret_cast<const char*>(fbb.GetBufferPointer()), fbb.GetSize());
+    return Async::start<void>([commandId, buffer, this](Async::Future<void> &f) {
         auto callback = [&f](int error, const QString &errorMessage) {
             if (error) {
                 f.setError(error, errorMessage);
@@ -169,12 +161,13 @@ Async::Job<void>  ResourceAccess::sendCommand(int commandId, flatbuffers::FlatBu
         };
 
         if (isReady()) {
+            //TODO: We probably always want to queue the command, so we can resend it in case something goes wrong
             d->messageId++;
             log(QString("Sending command %1 with messageId %2").arg(commandId).arg(d->messageId));
             registerCallback(d->messageId, callback);
-            Commands::write(d->socket, d->messageId, commandId, fbb);
+            Commands::write(d->socket, d->messageId, commandId, buffer.constData(), buffer.size());
         } else {
-            d->commandQueue << new QueuedCommand(commandId, fbb, callback);
+            d->commandQueue << new QueuedCommand(commandId, buffer, callback);
         }
     });
 }
@@ -237,7 +230,7 @@ void ResourceAccess::connected()
         if (command->callback) {
             registerCallback(d->messageId, command->callback);
         }
-        Commands::write(d->socket, d->messageId, command->commandId, command->buffer, command->bufferSize);
+        Commands::write(d->socket, d->messageId, command->commandId, command->buffer.constData(), command->buffer.size());
         delete command;
     }
     d->commandQueue.clear();
-- 
cgit v1.2.3