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

Proposed by Michi Henning
Status: Merged
Approved by: James Henstridge
Approved revision: 80
Merged at revision: 42
Proposed branch: lp:~michihenning/storage-framework/upload-download-fixes
Merge into: lp:storage-framework/devel
Prerequisite: lp:~michihenning/storage-framework/more-coverage
Diff against target: 334 lines (+115/-14)
10 files modified
include/unity/storage/qt/client/Uploader.h (+8/-0)
include/unity/storage/qt/client/internal/UploaderBase.h (+4/-1)
include/unity/storage/qt/client/internal/remote_client/UploaderImpl.h (+0/-1)
src/qt/client/Uploader.cpp (+5/-0)
src/qt/client/internal/UploaderBase.cpp (+11/-2)
src/qt/client/internal/local_client/UploaderImpl.cpp (+12/-5)
src/qt/client/internal/remote_client/DownloaderImpl.cpp (+4/-2)
src/qt/client/internal/remote_client/UploaderImpl.cpp (+4/-3)
tests/local-client/local-client_test.cpp (+65/-0)
tests/remote-client/remote-client_test.cpp (+2/-0)
To merge this branch: bzr merge lp:~michihenning/storage-framework/upload-download-fixes
Reviewer Review Type Date Requested Status
James Henstridge Approve
unity-api-1-bot continuous-integration Needs Fixing
Review via email: mp+301865@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
unity-api-1-bot (unity-api-1-bot) wrote :

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)
Revision history for this message
James Henstridge (jamesh) wrote :

Looks great!

Revision history for this message
James Henstridge (jamesh) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/unity/storage/qt/client/Uploader.h'
2--- include/unity/storage/qt/client/Uploader.h 2016-07-22 02:35:12 +0000
3+++ include/unity/storage/qt/client/Uploader.h 2016-08-03 04:16:38 +0000
4@@ -92,6 +92,14 @@
5 std::shared_ptr<QLocalSocket> socket() const;
6
7 /**
8+ \brief Returns the size that was passed to Folder::create_file() or File::create_uploader().
9+
10+ \return The number of bytes that the uploader expects to be written to the `QLocalSocket` returned
11+ from socket().
12+ */
13+ int64_t size() const;
14+
15+ /**
16 \brief Finalizes the upload.
17
18 Once you have written the file contents to the socket returned by socket(), you must call finish_upload(),
19
20=== modified file 'include/unity/storage/qt/client/internal/UploaderBase.h'
21--- include/unity/storage/qt/client/internal/UploaderBase.h 2016-07-22 02:35:12 +0000
22+++ include/unity/storage/qt/client/internal/UploaderBase.h 2016-08-03 04:16:38 +0000
23@@ -47,7 +47,7 @@
24 class UploaderBase : public QObject
25 {
26 public:
27- UploaderBase(ConflictPolicy policy);
28+ UploaderBase(ConflictPolicy policy, int64_t size);
29 UploaderBase(UploaderBase&) = delete;
30 UploaderBase& operator=(UploaderBase const&) = delete;
31
32@@ -55,8 +55,11 @@
33 virtual QFuture<std::shared_ptr<File>> finish_upload() = 0;
34 virtual QFuture<void> cancel() noexcept = 0;
35
36+ int64_t size() const;
37+
38 protected:
39 ConflictPolicy policy_;
40+ int64_t size_;
41 };
42
43 } // namespace internal
44
45=== modified file 'include/unity/storage/qt/client/internal/remote_client/UploaderImpl.h'
46--- include/unity/storage/qt/client/internal/remote_client/UploaderImpl.h 2016-08-03 04:16:38 +0000
47+++ include/unity/storage/qt/client/internal/remote_client/UploaderImpl.h 2016-08-03 04:16:38 +0000
48@@ -67,7 +67,6 @@
49 private:
50 QString upload_id_;
51 QDBusUnixFileDescriptor fd_;
52- int64_t size_;
53 QString old_etag_;
54 std::shared_ptr<Root> root_;
55 std::shared_ptr<ProviderInterface> provider_;
56
57=== modified file 'src/qt/client/Uploader.cpp'
58--- src/qt/client/Uploader.cpp 2016-07-12 02:22:05 +0000
59+++ src/qt/client/Uploader.cpp 2016-08-03 04:16:38 +0000
60@@ -46,6 +46,11 @@
61 return p_->socket();
62 }
63
64+int64_t Uploader::size() const
65+{
66+ return p_->size();
67+}
68+
69 QFuture<shared_ptr<File>> Uploader::finish_upload()
70 {
71 return p_->finish_upload();
72
73=== modified file 'src/qt/client/internal/UploaderBase.cpp'
74--- src/qt/client/internal/UploaderBase.cpp 2016-07-22 01:45:39 +0000
75+++ src/qt/client/internal/UploaderBase.cpp 2016-08-03 04:16:38 +0000
76@@ -25,6 +25,8 @@
77 #include <QFuture>
78 #pragma GCC diagnostic pop
79
80+#include <cassert>
81+
82 using namespace std;
83
84 namespace unity
85@@ -38,9 +40,16 @@
86 namespace internal
87 {
88
89-UploaderBase::UploaderBase(ConflictPolicy policy)
90+UploaderBase::UploaderBase(ConflictPolicy policy, int64_t size)
91 : policy_(policy)
92-{
93+ , size_(size)
94+{
95+ assert(size >= 0);
96+}
97+
98+int64_t UploaderBase::size() const
99+{
100+ return size_;
101 }
102
103 } // namespace internal
104
105=== modified file 'src/qt/client/internal/local_client/UploaderImpl.cpp'
106--- src/qt/client/internal/local_client/UploaderImpl.cpp 2016-08-03 04:16:38 +0000
107+++ src/qt/client/internal/local_client/UploaderImpl.cpp 2016-08-03 04:16:38 +0000
108@@ -143,8 +143,17 @@
109 {
110 case in_progress:
111 {
112- state_ = finalized;
113- finalize();
114+ if (bytes_read_ != size_)
115+ {
116+ QString msg = "Uploader::finish_upload(): " + path_ + ": upload size of " + QString::number(size_)
117+ + " does not match actual number of bytes read: " + QString::number(bytes_read_);
118+ make_exceptional_future(qf_, LogicException(msg));
119+ }
120+ else
121+ {
122+ state_ = finalized;
123+ finalize();
124+ }
125 break;
126 }
127 case finalized:
128@@ -342,7 +351,7 @@
129 QString const& path,
130 ConflictPolicy policy,
131 weak_ptr<Root> root)
132- : UploaderBase(policy)
133+ : UploaderBase(policy, size)
134 , write_socket_(new QLocalSocket)
135 {
136 // Set up socket pair.
137@@ -401,7 +410,6 @@
138 if (write_socket_->state() == QLocalSocket::ConnectedState)
139 {
140 write_socket_->disconnectFromServer();
141- write_socket_->close();
142 }
143 return qf_.future();
144 }
145@@ -409,7 +417,6 @@
146 QFuture<void> UploaderImpl::cancel() noexcept
147 {
148 Q_EMIT do_cancel();
149- upload_thread_->wait();
150 write_socket_->abort();
151 return qf_.future();
152 }
153
154=== modified file 'src/qt/client/internal/remote_client/DownloaderImpl.cpp'
155--- src/qt/client/internal/remote_client/DownloaderImpl.cpp 2016-07-15 03:56:14 +0000
156+++ src/qt/client/internal/remote_client/DownloaderImpl.cpp 2016-08-03 04:16:38 +0000
157@@ -20,6 +20,7 @@
158
159 #include "ProviderInterface.h"
160 #include <unity/storage/qt/client/Downloader.h>
161+#include <unity/storage/qt/client/File.h>
162 #include <unity/storage/qt/client/internal/remote_client/Handler.h>
163
164 #include <cassert>
165@@ -61,7 +62,7 @@
166
167 DownloaderImpl::~DownloaderImpl()
168 {
169- cancel();
170+ read_socket_->abort();
171 }
172
173 shared_ptr<File> DownloaderImpl::file() const
174@@ -90,7 +91,8 @@
175 QFuture<void> DownloaderImpl::cancel() noexcept
176 {
177 read_socket_->abort();
178- return make_ready_future();
179+ QString msg = "Downloader::finish_download(): download of " + file_->name() + " was cancelled";
180+ return make_exceptional_future(CancelledException(msg));
181 }
182
183 Downloader::SPtr DownloaderImpl::make_downloader(QString const& download_id,
184
185=== modified file 'src/qt/client/internal/remote_client/UploaderImpl.cpp'
186--- src/qt/client/internal/remote_client/UploaderImpl.cpp 2016-08-03 04:16:38 +0000
187+++ src/qt/client/internal/remote_client/UploaderImpl.cpp 2016-08-03 04:16:38 +0000
188@@ -46,10 +46,9 @@
189 QString const& old_etag,
190 weak_ptr<Root> root,
191 shared_ptr<ProviderInterface> const& provider)
192- : UploaderBase(old_etag == "" ? ConflictPolicy::overwrite : ConflictPolicy::error_if_conflict)
193+ : UploaderBase(old_etag == "" ? ConflictPolicy::overwrite : ConflictPolicy::error_if_conflict, size)
194 , upload_id_(upload_id)
195 , fd_(fd)
196- , size_(size)
197 , old_etag_(old_etag)
198 , root_(root.lock())
199 , provider_(provider)
200@@ -66,7 +65,7 @@
201
202 UploaderImpl::~UploaderImpl()
203 {
204- cancel();
205+ write_socket_->abort();
206 }
207
208 shared_ptr<QLocalSocket> UploaderImpl::socket() const
209@@ -91,6 +90,7 @@
210 make_ready_future(qf, FileImpl::make_file(md, root_));
211 };
212
213+ write_socket_->disconnectFromServer();
214 auto handler = new Handler<shared_ptr<File>>(this, reply, process_reply);
215 return handler->future();
216 }
217@@ -103,6 +103,7 @@
218 make_ready_future();
219 };
220
221+ write_socket_->abort();
222 auto handler = new Handler<void>(this, reply, process_reply);
223 return handler->future();
224 }
225
226=== modified file 'tests/local-client/local-client_test.cpp'
227--- tests/local-client/local-client_test.cpp 2016-08-03 04:16:38 +0000
228+++ tests/local-client/local-client_test.cpp 2016-08-03 04:16:38 +0000
229@@ -422,6 +422,7 @@
230
231 // Make a new file first.
232 auto uploader = call(root->create_file("new_file", 0));
233+ EXPECT_EQ(0, uploader->size());
234 auto file = call(uploader->finish_upload());
235 EXPECT_EQ(0, file->size());
236 auto old_etag = file->etag();
237@@ -434,6 +435,7 @@
238 // Same test again, but this time, we write a bunch of data.
239 std::string s(1000000, 'a');
240 uploader = call(file->create_uploader(ConflictPolicy::overwrite, s.size()));
241+ EXPECT_EQ(1000000, uploader->size());
242 uploader->socket()->write(&s[0], s.size());
243
244 // Need to sleep here, otherwise it is possible for the
245@@ -549,6 +551,69 @@
246 }
247 }
248
249+TEST(File, upload_bad_size)
250+{
251+ auto runtime = Runtime::create();
252+
253+ auto acc = get_account(runtime);
254+ auto root = get_root(runtime);
255+ clear_folder(root);
256+
257+ // Uploader expects 100 bytes, but we write only 50.
258+ {
259+ auto uploader = call(root->create_file("file50", 100));
260+ auto socket = uploader->socket();
261+
262+ QByteArray const contents(50, 'x');
263+ auto written = socket->write(contents);
264+ ASSERT_EQ(50, written);
265+
266+ try
267+ {
268+ call(uploader->finish_upload());
269+ FAIL();
270+ }
271+ catch (LogicException const& e)
272+ {
273+ EXPECT_TRUE(e.error_message().startsWith("Uploader::finish_upload(): "));
274+ EXPECT_TRUE(e.error_message().endsWith(": upload size of 100 does not match actual number of bytes read: 50"));
275+ }
276+ }
277+
278+ // Uploader expects 100 bytes, but we write 101.
279+ {
280+ auto uploader = call(root->create_file("file100", 100));
281+ auto socket = uploader->socket();
282+
283+ QByteArray const contents(101, 'x');
284+ auto written = socket->write(contents);
285+ ASSERT_EQ(101, written);
286+
287+ try
288+ {
289+ call(uploader->finish_upload());
290+ FAIL();
291+ }
292+ catch (LogicException const& e)
293+ {
294+ EXPECT_TRUE(e.error_message().startsWith("Uploader::finish_upload(): "));
295+ EXPECT_TRUE(e.error_message().endsWith(": upload size of 100 does not match actual number of bytes read: 101"));
296+ }
297+
298+ // Calling finish_upload() again must return the same future as the first time.
299+ try
300+ {
301+ call(uploader->finish_upload());
302+ FAIL();
303+ }
304+ catch (LogicException const& e)
305+ {
306+ EXPECT_TRUE(e.error_message().startsWith("Uploader::finish_upload(): "));
307+ EXPECT_TRUE(e.error_message().endsWith(": upload size of 100 does not match actual number of bytes read: 101"));
308+ }
309+ }
310+}
311+
312 TEST(File, create_uploader_bad_arg)
313 {
314 auto runtime = Runtime::create();
315
316=== modified file 'tests/remote-client/remote-client_test.cpp'
317--- tests/remote-client/remote-client_test.cpp 2016-08-03 04:16:38 +0000
318+++ tests/remote-client/remote-client_test.cpp 2016-08-03 04:16:38 +0000
319@@ -276,6 +276,7 @@
320 ASSERT_TRUE(spy.wait(SIGNAL_WAIT_TIME));
321 }
322 auto uploader = create_file_fut.result();
323+ EXPECT_EQ(0, uploader->size());
324 auto finish_upload_fut = uploader->finish_upload();
325 {
326 QFutureWatcher<File::SPtr> w;
327@@ -424,6 +425,7 @@
328 ASSERT_TRUE(spy.wait(SIGNAL_WAIT_TIME));
329 }
330 auto uploader = create_uploader_fut.result();
331+ EXPECT_EQ(0, uploader->size());
332
333 auto finish_upload_fut = uploader->finish_upload();
334 {

Subscribers

People subscribed via source and target branches

to all changes: