Merge lp:~michihenning/storage-framework/upload-download-fixes into lp:storage-framework/devel

[Michi Henning]

Added size() accessor to Uploader. Fixed upload/download finalization for remote client.

Added size() accessor to Uploader.
Added tests for upload size mismatch and that calling finish_upload() more than once continues to return the same error future.
Fixed upload/download finalization for remote client.

[unity-api-1-bot (unity-api-1-bot)]

FAILED: Continuous integration, rev:80
https://jenkins.canonical.com/unity-api-1/job/lp-storage-framework-ci/46/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build/273/console
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/279
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-1-sourcepkg/release=vivid+overlay/213
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-1-sourcepkg/release=xenial+overlay/213
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-1-sourcepkg/release=yakkety/213
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=vivid+overlay/142/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/142/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=yakkety/142/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=vivid+overlay/142/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/142/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=yakkety/142/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=vivid+overlay/142/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/142/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=yakkety/142/console

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-storage-framework-ci/46/rebuild

review: Needs Fixing (continuous-integration)

[James Henstridge (jamesh)]

Looks great!

[James Henstridge (jamesh)]

review: Approve

=== modified file 'include/unity/storage/qt/client/Uploader.h'
--- include/unity/storage/qt/client/Uploader.h	2016-07-22 02:35:12 +0000
+++ include/unity/storage/qt/client/Uploader.h	2016-08-03 04:16:38 +0000
@@ -92,6 +92,14 @@
     std::shared_ptr<QLocalSocket> socket() const;
 
     /**
+    \brief Returns the size that was passed to Folder::create_file() or File::create_uploader().
+
+    \return The number of bytes that the uploader expects to be written to the `QLocalSocket` returned
+    from socket().
+    */
+    int64_t size() const;
+
+    /**
     \brief Finalizes the upload.
 
     Once you have written the file contents to the socket returned by socket(), you must call finish_upload(),
 
=== modified file 'include/unity/storage/qt/client/internal/UploaderBase.h'
--- include/unity/storage/qt/client/internal/UploaderBase.h	2016-07-22 02:35:12 +0000
+++ include/unity/storage/qt/client/internal/UploaderBase.h	2016-08-03 04:16:38 +0000
@@ -47,7 +47,7 @@
 class UploaderBase : public QObject
 {
 public:
-    UploaderBase(ConflictPolicy policy);
+    UploaderBase(ConflictPolicy policy, int64_t size);
     UploaderBase(UploaderBase&) = delete;
     UploaderBase& operator=(UploaderBase const&) = delete;
 
@@ -55,8 +55,11 @@
     virtual QFuture<std::shared_ptr<File>> finish_upload() = 0;
     virtual QFuture<void> cancel() noexcept = 0;
 
+    int64_t size() const;
+
 protected:
     ConflictPolicy policy_;
+    int64_t size_;
 };
 
 }  // namespace internal
 
=== modified file 'include/unity/storage/qt/client/internal/remote_client/UploaderImpl.h'
--- include/unity/storage/qt/client/internal/remote_client/UploaderImpl.h	2016-08-03 04:16:38 +0000
+++ include/unity/storage/qt/client/internal/remote_client/UploaderImpl.h	2016-08-03 04:16:38 +0000
@@ -67,7 +67,6 @@
 private:
     QString upload_id_;
     QDBusUnixFileDescriptor fd_;
-    int64_t size_;
     QString old_etag_;
     std::shared_ptr<Root> root_;
     std::shared_ptr<ProviderInterface> provider_;
 
=== modified file 'src/qt/client/Uploader.cpp'
--- src/qt/client/Uploader.cpp	2016-07-12 02:22:05 +0000
+++ src/qt/client/Uploader.cpp	2016-08-03 04:16:38 +0000
@@ -46,6 +46,11 @@
     return p_->socket();
 }
 
+int64_t Uploader::size() const
+{
+    return p_->size();
+}
+
 QFuture<shared_ptr<File>> Uploader::finish_upload()
 {
     return p_->finish_upload();
 
=== modified file 'src/qt/client/internal/UploaderBase.cpp'
--- src/qt/client/internal/UploaderBase.cpp	2016-07-22 01:45:39 +0000
+++ src/qt/client/internal/UploaderBase.cpp	2016-08-03 04:16:38 +0000
@@ -25,6 +25,8 @@
 #include <QFuture>
 #pragma GCC diagnostic pop
 
+#include <cassert>
+
 using namespace std;
 
 namespace unity
@@ -38,9 +40,16 @@
 namespace internal
 {
 
-UploaderBase::UploaderBase(ConflictPolicy policy)
+UploaderBase::UploaderBase(ConflictPolicy policy, int64_t size)
     : policy_(policy)
-{
+    , size_(size)
+{
+    assert(size >= 0);
+}
+
+int64_t UploaderBase::size() const
+{
+    return size_;
 }
 
 }  // namespace internal
 
=== modified file 'src/qt/client/internal/local_client/UploaderImpl.cpp'
--- src/qt/client/internal/local_client/UploaderImpl.cpp	2016-08-03 04:16:38 +0000
+++ src/qt/client/internal/local_client/UploaderImpl.cpp	2016-08-03 04:16:38 +0000
@@ -143,8 +143,17 @@
     {
         case in_progress:
         {
-            state_ = finalized;
-            finalize();
+            if (bytes_read_ != size_)
+            {
+                QString msg = "Uploader::finish_upload(): " + path_ + ": upload size of " + QString::number(size_)
+                              + " does not match actual number of bytes read: " + QString::number(bytes_read_);
+                make_exceptional_future(qf_, LogicException(msg));
+            }
+            else
+            {
+                state_ = finalized;
+                finalize();
+            }
             break;
         }
         case finalized:
@@ -342,7 +351,7 @@
                            QString const& path,
                            ConflictPolicy policy,
                            weak_ptr<Root> root)
-    : UploaderBase(policy)
+    : UploaderBase(policy, size)
     , write_socket_(new QLocalSocket)
 {
     // Set up socket pair.
@@ -401,7 +410,6 @@
     if (write_socket_->state() == QLocalSocket::ConnectedState)
     {
         write_socket_->disconnectFromServer();
-        write_socket_->close();
     }
     return qf_.future();
 }
@@ -409,7 +417,6 @@
 QFuture<void> UploaderImpl::cancel() noexcept
 {
     Q_EMIT do_cancel();
-    upload_thread_->wait();
     write_socket_->abort();
     return qf_.future();
 }
 
=== modified file 'src/qt/client/internal/remote_client/DownloaderImpl.cpp'
--- src/qt/client/internal/remote_client/DownloaderImpl.cpp	2016-07-15 03:56:14 +0000
+++ src/qt/client/internal/remote_client/DownloaderImpl.cpp	2016-08-03 04:16:38 +0000
@@ -20,6 +20,7 @@
 
 #include "ProviderInterface.h"
 #include <unity/storage/qt/client/Downloader.h>
+#include <unity/storage/qt/client/File.h>
 #include <unity/storage/qt/client/internal/remote_client/Handler.h>
 
 #include <cassert>
@@ -61,7 +62,7 @@
 
 DownloaderImpl::~DownloaderImpl()
 {
-    cancel();
+    read_socket_->abort();
 }
 
 shared_ptr<File> DownloaderImpl::file() const
@@ -90,7 +91,8 @@
 QFuture<void> DownloaderImpl::cancel() noexcept
 {
     read_socket_->abort();
-    return make_ready_future();
+    QString msg = "Downloader::finish_download(): download of " + file_->name() + " was cancelled";
+    return make_exceptional_future(CancelledException(msg));
 }
 
 Downloader::SPtr DownloaderImpl::make_downloader(QString const& download_id,
 
=== modified file 'src/qt/client/internal/remote_client/UploaderImpl.cpp'
--- src/qt/client/internal/remote_client/UploaderImpl.cpp	2016-08-03 04:16:38 +0000
+++ src/qt/client/internal/remote_client/UploaderImpl.cpp	2016-08-03 04:16:38 +0000
@@ -46,10 +46,9 @@
                            QString const& old_etag,
                            weak_ptr<Root> root,
                            shared_ptr<ProviderInterface> const& provider)
-    : UploaderBase(old_etag == "" ? ConflictPolicy::overwrite : ConflictPolicy::error_if_conflict)
+    : UploaderBase(old_etag == "" ? ConflictPolicy::overwrite : ConflictPolicy::error_if_conflict, size)
     , upload_id_(upload_id)
     , fd_(fd)
-    , size_(size)
     , old_etag_(old_etag)
     , root_(root.lock())
     , provider_(provider)
@@ -66,7 +65,7 @@
 
 UploaderImpl::~UploaderImpl()
 {
-    cancel();
+    write_socket_->abort();
 }
 
 shared_ptr<QLocalSocket> UploaderImpl::socket() const
@@ -91,6 +90,7 @@
         make_ready_future(qf, FileImpl::make_file(md, root_));
     };
 
+    write_socket_->disconnectFromServer();
     auto handler = new Handler<shared_ptr<File>>(this, reply, process_reply);
     return handler->future();
 }
@@ -103,6 +103,7 @@
         make_ready_future();
     };
 
+    write_socket_->abort();
     auto handler = new Handler<void>(this, reply, process_reply);
     return handler->future();
 }
 
=== modified file 'tests/local-client/local-client_test.cpp'
--- tests/local-client/local-client_test.cpp	2016-08-03 04:16:38 +0000
+++ tests/local-client/local-client_test.cpp	2016-08-03 04:16:38 +0000
@@ -422,6 +422,7 @@
 
     // Make a new file first.
     auto uploader = call(root->create_file("new_file", 0));
+    EXPECT_EQ(0, uploader->size());
     auto file = call(uploader->finish_upload());
     EXPECT_EQ(0, file->size());
     auto old_etag = file->etag();
@@ -434,6 +435,7 @@
     // Same test again, but this time, we write a bunch of data.
     std::string s(1000000, 'a');
     uploader = call(file->create_uploader(ConflictPolicy::overwrite, s.size()));
+    EXPECT_EQ(1000000, uploader->size());
     uploader->socket()->write(&s[0], s.size());
 
     // Need to sleep here, otherwise it is possible for the
@@ -549,6 +551,69 @@
     }
 }
 
+TEST(File, upload_bad_size)
+{
+    auto runtime = Runtime::create();
+
+    auto acc = get_account(runtime);
+    auto root = get_root(runtime);
+    clear_folder(root);
+
+    // Uploader expects 100 bytes, but we write only 50.
+    {
+        auto uploader = call(root->create_file("file50", 100));
+        auto socket = uploader->socket();
+
+        QByteArray const contents(50, 'x');
+        auto written = socket->write(contents);
+        ASSERT_EQ(50, written);
+
+        try
+        {
+            call(uploader->finish_upload());
+            FAIL();
+        }
+        catch (LogicException const& e)
+        {
+            EXPECT_TRUE(e.error_message().startsWith("Uploader::finish_upload(): "));
+            EXPECT_TRUE(e.error_message().endsWith(": upload size of 100 does not match actual number of bytes read: 50"));
+        }
+    }
+
+    // Uploader expects 100 bytes, but we write 101.
+    {
+        auto uploader = call(root->create_file("file100", 100));
+        auto socket = uploader->socket();
+
+        QByteArray const contents(101, 'x');
+        auto written = socket->write(contents);
+        ASSERT_EQ(101, written);
+
+        try
+        {
+            call(uploader->finish_upload());
+            FAIL();
+        }
+        catch (LogicException const& e)
+        {
+            EXPECT_TRUE(e.error_message().startsWith("Uploader::finish_upload(): "));
+            EXPECT_TRUE(e.error_message().endsWith(": upload size of 100 does not match actual number of bytes read: 101"));
+        }
+
+        // Calling finish_upload() again must return the same future as the first time.
+        try
+        {
+            call(uploader->finish_upload());
+            FAIL();
+        }
+        catch (LogicException const& e)
+        {
+            EXPECT_TRUE(e.error_message().startsWith("Uploader::finish_upload(): "));
+            EXPECT_TRUE(e.error_message().endsWith(": upload size of 100 does not match actual number of bytes read: 101"));
+        }
+    }
+}
+
 TEST(File, create_uploader_bad_arg)
 {
     auto runtime = Runtime::create();
 
=== modified file 'tests/remote-client/remote-client_test.cpp'
--- tests/remote-client/remote-client_test.cpp	2016-08-03 04:16:38 +0000
+++ tests/remote-client/remote-client_test.cpp	2016-08-03 04:16:38 +0000
@@ -276,6 +276,7 @@
         ASSERT_TRUE(spy.wait(SIGNAL_WAIT_TIME));
     }
     auto uploader = create_file_fut.result();
+    EXPECT_EQ(0, uploader->size());
     auto finish_upload_fut = uploader->finish_upload();
     {
         QFutureWatcher<File::SPtr> w;
@@ -424,6 +425,7 @@
         ASSERT_TRUE(spy.wait(SIGNAL_WAIT_TIME));
     }
     auto uploader = create_uploader_fut.result();
+    EXPECT_EQ(0, uploader->size());
 
     auto finish_upload_fut = uploader->finish_upload();
     {