Fixes for fetcher #1032

eu-siemann · 2018-12-11T13:18:00Z

There are several problems with pause-resume fetcher API and the corresponding fetcher test that I see at the moment.

Fetcher test currently uses large_interrupted file for pause/resume. With large_interrupted server aborts after transferring half of a file (1000 bytes). This should lead to the The target's calculated hash did not match the hash in the metadata error and, after that, the upper layer may choose to retry. However, currently, the test succeeds, because while committing the first half to the db, fetcher gets pause event, which makes it retry silently after resume is called. This behavior itself is not a problem, the problem is that it is also correct for the test to fail, if pause comes after the first chunk is committed.

Race condition between fetchVerifyTarget() or DownloadHandler() and setPause():

http->download()
                                            setPause(true) 
DownloadHandler() {
...
return written_size + 1
                                            setPause(false)
http->download() == CURLE_WRITE_ERROR && 
pause_ == false
throw OversizedTarget

Using a mutex to pause the fetcher is not only non-idiomatic, but it is also an undefined behavior, as the thread which called pause may terminate and/or resume might get called from another thread. See [https://en.cppreference.com/w/cpp/thread/mutex] and [https://en.cppreference.com/w/cpp/thread/mutex/unlock]. It seems to work now, but it can easily break anytime.
The downloading_ guard in fetcher is pretty useless IMO. If I understand correctly its main purpose is to return kNotDownloading, if the Download API wasn't called yet. But the downloading_ flag
doesn't guarantee anything. The high-level Download API call is asynchronous and the execution of underlying Fetcher::fetchVerifyTarget will be performed at some point in the future on the command queue. Pause, on the other hand, is synchronous, so it's entirely possible that caller gets a kNotDownloading even after he has called Download. That means that caller should track the downloading state himself and provide some retry logic for the aforementioned case.

So why bother with the kNotDownloading at all?
Cleaner approach IMO would be to set pause no matter if the Download was called or not. In case Download called after a Pause it should just block until the Resume is called.

This PR attempts to solve issues 1-3, issue 4 is out of scope currently, I just want to start a discussion on it.

codecov-io · 2018-12-11T14:01:57Z

Codecov Report

Merging #1032 into master will increase coverage by 0.01%.
The diff coverage is 97.77%.

@@            Coverage Diff             @@
##           master    #1032      +/-   ##
==========================================
+ Coverage   82.31%   82.33%   +0.01%     
==========================================
  Files         189      189              
  Lines       13663    13671       +8     
==========================================
+ Hits        11247    11256       +9     
+ Misses       2416     2415       -1

Impacted Files	Coverage Δ
src/libaktualizr/package_manager/ostreemanager.h	`83.33% <ø> (-2.39%)`	⬇️
src/libaktualizr/http/httpclient.h	`100% <ø> (ø)`	⬆️
src/libaktualizr/uptane/fetcher.h	`86.66% <100%> (+0.45%)`	⬆️
src/libaktualizr/package_manager/ostreemanager.cc	`54.78% <100%> (+0.08%)`	⬆️
src/libaktualizr/http/httpinterface.h	`100% <100%> (ø)`	⬆️
src/libaktualizr/http/httpclient.cc	`91.13% <100%> (ø)`	⬆️
src/libaktualizr/uptane/fetcher.cc	`96.93% <97.22%> (-0.24%)`	⬇️
src/libaktualizr/uptane/fetcher_test.cc	`96.52% <97.61%> (+0.6%)`	⬆️
src/libaktualizr/primary/reportqueue_test.cc	`95.94% <0%> (-1.36%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5d105c...816c81e. Read the comment docs.

lbonn · 2018-12-11T14:59:15Z

src/libaktualizr/package_manager/ostreemanager.cc

-  } else {
-    mt->pause_mutex->unlock();  // Unlock locked by try_lock() mutex.
-  }
+  mt->pause();


Can we add a comment here, to document that we expect to wait for the download to not be paused anymore?

This callback is passed through several method calls and constructors and the intent is a bit obfuscated here, IMO.

Agree, also the name is a bit confusing, as it will not pause on every call, but I wasn't able to come up with something better. I'll add a comment.

+1 for a comment, but even just checkPause would be a better name.

True, I'll rename it.

Sorry, a bit late again. But it probably wouldn't hurt to have a null check as well before making the call.

@lbonn I don't know, std:function will throw in case it's empty. Do we want to proceed if pause callback isn't provided?

Just having:

if (checkPause != nullptr) { checkPause() }

Then, users of the class could just pass a null pointer if they don't need the pause functionality. It's only an added functionality, not a part of the core duty of the class.

Agreed, that would make sense.

src/libaktualizr/http/httpinterface.h

OYTIS

Apart from what is already mentioned, looks good to me. I never liked that mutex.

pattivacek

A lot of good stuff here. Thanks!

pattivacek · 2018-12-12T09:30:02Z

src/libaktualizr/package_manager/ostreemanager.cc

-  } else {
-    mt->pause_mutex->unlock();  // Unlock locked by try_lock() mutex.
-  }
+  mt->pause();


+1 for a comment, but even just checkPause would be a better name.

pattivacek · 2018-12-12T09:47:14Z

src/libaktualizr/uptane/fetcher.cc

        }
-      } while (retry);
+        throw Exception("image", "Could not download file, error: " + response.error_message);
+      }


This cleanup is definitely an improvement, but I think we are losing our retry functionality. We currently retry downloads at a high level if they fail for basically any reason. I think that's worth keeping. Originally, I think we gave it three tries before failing.

Thanks! I forgot to look into sotauptaneclient.cc. The retry indeed will not work. But does it work now? The code on master throws in exactly the same way.

My bad, the actual retry logic is in SotaUptaneClient::downloadImage. All is well, sorry!

src/libaktualizr/uptane/fetcher_test.cc

Fetcher test currently uses `large_interrupted` file for pause/resume. With `large_interrupted` server aborts after transferring half of a file (1000 bytes). This should lead to the `The target's calculated hash did not match the hash in the metadata` error and after that, the upper layer may choose to retry. However, currently, the test succeeds, because while committing the first half to the db, fetcher gets `pause` event, which makes it retry silently after `resume` is called. This behavior itself is not a problem, the problem is that it is also correct for the test to fail, if `pause` comes after the first chunk is committed. To solve this: * Use `large_file` * Launch download in a separate thread, so we can timeout, currently after 20 sec. * Pause the fetcher after a certain percent is downloaded, rather than after 200 ms. Signed-off-by: Eugene Smirnov <[email protected]>

We cannot get any progress report on a file of 2000 bytes, as we receive all file in a single curl callback. Signed-off-by: Eugene Smirnov <[email protected]>

eu-siemann · 2018-12-13T16:29:42Z

I've rebased this to the merged sqlite changes, added couple of small fixes from my previous abandoned PR, and resolved the comments, except the one, which I'm not yet sure about.

OYTIS · 2018-12-13T16:57:34Z

src/libaktualizr/http/httpclient.cc

@@ -234,7 +234,7 @@ std::future<HttpResponse> HttpClient::downloadAsync(const std::string& url, curl
  curlEasySetoptWrapper(curl_download, CURLOPT_TIMEOUT, 0);
  curlEasySetoptWrapper(curl_download, CURLOPT_LOW_SPEED_TIME, speed_limit_time_interval_);
  curlEasySetoptWrapper(curl_download, CURLOPT_LOW_SPEED_LIMIT, speed_limit_bytes_per_sec_);
-  curlEasySetoptWrapper(curl_download, CURLOPT_RESUME_FROM, from);
+  curlEasySetoptWrapper(curl_download, CURLOPT_RESUME_FROM_LARGE, from);


Would it make sense then to change the type of from to curl_off_t?

Yes, absolutely. Thanks!

Using a mutex to pause the fetcher is not only non-idiomatic, but it is also an undefined behavior, as the thread which called `pause` may terminate and/or resume might get called from another thread. See [https://en.cppreference.com/w/cpp/thread/mutex] and [https://en.cppreference.com/w/cpp/thread/mutex/unlock]. It seems to work now, but it can easily break anytime. Also there was a race condition between `fetchVerifyTarget()` or `DownloadHandler()` and `setPause()`, which should be solved with this commit: http->download() setPause(true) DownloadHandler() { ... return written_size + 1 setPause(false) http->download() == CURLE_WRITE_ERROR && pause_ == false throw OversizedTarget Signed-off-by: Eugene Smirnov <[email protected]>

* Remove prints that are duplicated in hmi_stub/tests * Don't send progress events with the same percentage. Signed-off-by: Eugene Smirnov <[email protected]>

Signed-off-by: Eugene Smirnov <[email protected]>

eu-siemann requested review from patriotyk, pattivacek, lbonn, OYTIS and xaviergully December 11, 2018 13:18

eu-siemann force-pushed the fix/pause-resume-race branch from 5e38e08 to b39eaa3 Compare December 11, 2018 14:01

lbonn reviewed Dec 11, 2018

View reviewed changes

OYTIS reviewed Dec 11, 2018

View reviewed changes

src/libaktualizr/http/httpinterface.h Outdated Show resolved Hide resolved

OYTIS approved these changes Dec 11, 2018

View reviewed changes

pattivacek reviewed Dec 12, 2018

View reviewed changes

OYTIS mentioned this pull request Dec 13, 2018

Serialize downloads #1031

Merged

Eugene Smirnov added 2 commits December 13, 2018 17:01

Tests: make large_file larger

627150b

We cannot get any progress report on a file of 2000 bytes, as we receive all file in a single curl callback. Signed-off-by: Eugene Smirnov <[email protected]>

eu-siemann force-pushed the fix/pause-resume-race branch 2 times, most recently from 9d8ba4f to a16bb89 Compare December 13, 2018 16:15

eu-siemann force-pushed the fix/pause-resume-race branch from a16bb89 to 16f331e Compare December 13, 2018 16:43

OYTIS reviewed Dec 13, 2018

View reviewed changes

Eugene Smirnov added 4 commits December 13, 2018 18:13

Fetcher: cleanup download progress reporting

1e6bb1b

* Remove prints that are duplicated in hmi_stub/tests * Don't send progress events with the same percentage. Signed-off-by: Eugene Smirnov <[email protected]>

Tests: get rid of HTTP/1.0 501 Unsupported method ('HEAD') error

f9f8aea

Signed-off-by: Eugene Smirnov <[email protected]>

HTTPClient: support resuming files > 2GB

816c81e

Signed-off-by: Eugene Smirnov <[email protected]>

eu-siemann force-pushed the fix/pause-resume-race branch from 16f331e to 816c81e Compare December 13, 2018 17:30

lbonn approved these changes Dec 14, 2018

View reviewed changes

OYTIS merged commit a39ed7b into master Dec 14, 2018

eu-siemann deleted the fix/pause-resume-race branch December 14, 2018 09:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for fetcher #1032

Fixes for fetcher #1032

eu-siemann commented Dec 11, 2018

codecov-io commented Dec 11, 2018 •

edited

Loading

lbonn Dec 11, 2018

eu-siemann Dec 11, 2018

pattivacek Dec 12, 2018

eu-siemann Dec 12, 2018

lbonn Dec 12, 2018

eu-siemann Dec 13, 2018

lbonn Dec 13, 2018

eu-siemann Dec 13, 2018

OYTIS left a comment

pattivacek left a comment

pattivacek Dec 12, 2018

pattivacek Dec 12, 2018

eu-siemann Dec 12, 2018

pattivacek Dec 12, 2018

eu-siemann commented Dec 13, 2018

OYTIS Dec 13, 2018 •

edited

Loading

eu-siemann Dec 13, 2018

Fixes for fetcher #1032

Fixes for fetcher #1032

Conversation

eu-siemann commented Dec 11, 2018

codecov-io commented Dec 11, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OYTIS left a comment

Choose a reason for hiding this comment

pattivacek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eu-siemann commented Dec 13, 2018

OYTIS Dec 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Dec 11, 2018 •

edited

Loading

OYTIS Dec 13, 2018 •

edited

Loading