[Offline pages] Introduction of MarkAttemptStartedTask with tests

components/offline_pages/background/request_coordinator.cc

Index: components/offline_pages/background/request_coordinator.cc
diff --git a/components/offline_pages/background/request_coordinator.cc b/components/offline_pages/background/request_coordinator.cc
index 087be6fa282ab13431bbe9e88b9f980e9117a73a..ba9ec510a9333f87d30dee39fc78a46694a7837f 100644
--- a/components/offline_pages/background/request_coordinator.cc
+++ b/components/offline_pages/background/request_coordinator.cc
@@ -538,18 +538,41 @@ void RequestCoordinator::SendRequestToOffliner(const SavePageRequest& request) {
   is_busy_ = true;
 
   // Update the request for this attempt to start it.

[Pete Williamson, 2016/10/14 17:02:35]

Comment needs to be updated to say "and then send it to the offliner"

[fgorski, 2016/10/19 03:26:33]

Done.

-  SavePageRequest updated_request(request);
-  updated_request.MarkAttemptStarted(base::Time::Now());
-  queue_->UpdateRequest(
-      updated_request,
-      base::Bind(&RequestCoordinator::UpdateRequestCallback,
-                 weak_ptr_factory_.GetWeakPtr(), updated_request.client_id()));
-  active_request_.reset(new SavePageRequest(updated_request));
+  queue_->MarkAttemptStarted(
+      request.request_id(),
+      base::Bind(&RequestCoordinator::StartOffliner,
+                 weak_ptr_factory_.GetWeakPtr(), request.request_id(),
+                 request.client_id().name_space));
+}
+
+void RequestCoordinator::StartOffliner(
+    int64_t request_id,
+    const std::string& client_namespace,
+    std::unique_ptr<UpdateRequestsResult> update_result) {
+  if (update_result->store_state != StoreState::LOADED ||

[Pete Williamson, 2016/10/14 17:02:35]

I think our original decision was to keep processing even if the request could
not be marked as started.  Marking as started serves primarily to prevent crash
loops, and this failing should be extremely rare.

The primary causes of this failing would be if the request database were
unavailable, or the request was not found.  However, in either case, we would
not have found the request to start processing on it.  Maybe logging here is
enough?

[fgorski, 2016/10/19 03:26:34]

What if request is not in database? What if the store is corrupted?
This catches such cases.

+      update_result->item_statuses.size() != 1 ||
+      update_result->item_statuses.at(0).first != request_id ||
+      update_result->item_statuses.at(0).second != ItemActionStatus::SUCCESS) {
+    is_busy_ = false;
+    // TODO(fgorski): what is the best result? Do we create a new status?
+    StopProcessing(Offliner::PRERENDERING_NOT_STARTED);
+    DVLOG(1) << "Failed to mark attempt started: " << request_id;
+    event_logger_.RecordUpdateRequestFailed(
+        client_namespace,
+        update_result->store_state != StoreState::LOADED
+            ? RequestQueue::UpdateRequestResult::STORE_FAILURE
+            : RequestQueue::UpdateRequestResult::REQUEST_DOES_NOT_EXIST);
+  }
+
+  // TODO(fgorski): Drop the in-memory caching.

[Pete Williamson, 2016/10/14 17:02:35]

I don't understand this comment.  Are you suggesting we deprecate the in-memory
store?  Unit tests still use that, but they could presumably be switched to the
SQL store.

[fgorski, 2016/10/14 20:37:19]

this is explicitly about active_request_ which is a single item in memory
cache... the in memory store stays based on your comment above.

[dougarnett, 2016/10/14 20:59:04]

Yes, we want to set active_request_ if we call LoadAndSave (or especially if it
returns true). I am confused at the moment though why we might want it set if
LoadAndSave returns false. Pete, can you think if this is intentional?

[fgorski, 2016/10/17 14:22:13]

Doug, you confirmed what the code does right now, but you are not explaining why
we need that and it's really the most important part. Please, explain.

[dougarnett, 2016/10/17 16:36:58]

It tracks state of pending operation and which request it is for potential
canceling or aborting and UMA. The request_id is used to decide whether a 
Paused or Removed request should cause Offliner to be canceled. The namespace,
client_id, and creation_time are use for UMA. The request itself is used for
MarkAttemptAborted. So using static information plus handle to make that
MarkAttemptAborted call. If we can put the mark aborted and uma within a Task,
them probably could just cache request_id.

[dougarnett, 2016/10/17 18:59:44]

Please clarify the TODO wrt your idea of only caching the request_id instead
of the full request so we are more clear about the idea. We can later figure
out how to deal with caching or querying the details needed for UMA when we
look to DO the TODO.

[fgorski, 2016/10/19 03:26:33]

Done.

+  active_request_.reset(
+      new SavePageRequest(update_result->updated_items.at(0)));
 
   // Start the load and save process in the offliner (Async).
   if (offliner_->LoadAndSave(
-          updated_request, base::Bind(&RequestCoordinator::OfflinerDoneCallback,
-                                      weak_ptr_factory_.GetWeakPtr()))) {
+          update_result->updated_items.at(0),
+          base::Bind(&RequestCoordinator::OfflinerDoneCallback,
+                     weak_ptr_factory_.GetWeakPtr()))) {
     // Start a watchdog timer to catch pre-renders running too long
     watchdog_timer_.Start(FROM_HERE, offliner_timeout_, this,
                           &RequestCoordinator::HandleWatchdogTimeout);
@@ -557,6 +580,8 @@ void RequestCoordinator::SendRequestToOffliner(const SavePageRequest& request) {
     is_busy_ = false;
     DVLOG(0) << "Unable to start LoadAndSave";
     StopProcessing(Offliner::PRERENDERING_NOT_STARTED);
+    // TODO(fgorski): We should probably mark attempt failed here. How is that

[dougarnett, 2016/10/13 22:54:56]

Unfortunately we do not know if it is a failure or not (PrerenderManager doesn't
tell us).

Possibly it just was not accepted at this time (previously this was most common
for the RECENTLY_VISITED check - but we have resolved that one). Couple other
cases we conceivable could hit are TOO_MANY_PROCESSES and RATE_LIMIT_EXCEEDED.
So we just count as a start attempt up above for now so that can be retried but
not indefinitely.

[fgorski, 2016/10/14 20:37:19]

Yeah, thanks for the explanation. I spoke with Pete after sending a patch and he
explained the same thing, which leads me to simply removing the TODO. BTW. I
appreciate the fact that this is a solid way to communicate with reviewers ;)

[fgorski, 2016/10/19 03:26:34]

Done.

+    // done if the offliner does not accept the task?
   }
 }