[fsp] Initial implementation of timeout logic for requests.

chrome/browser/chromeos/file_system_provider/request_manager.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/file_system_provider/request_manager.h"
 
+#include "base/files/file.h"
 #include "base/values.h"
 
 namespace chromeos {
 namespace file_system_provider {
 
-RequestManager::RequestManager() : next_id_(1) {}
+namespace {
+
+// Timeout in milliseconds, before a request is considered as stale and hence
+// aborted.
+const int kDefaultTimeout = 10000;  // 10 seconds.
+
+}  // namespace
+
+RequestManager::RequestManager() : next_id_(1), timeout_(kTimeout) {}

[hashimoto, 2014/04/18 05:28:09]

Does this line compile?

[mtomasz, 2014/04/18 05:56:18]

Oops. Done.

 
 RequestManager::~RequestManager() {
   // Abort all of the active requests.
   RequestMap::iterator it = requests_.begin();
   while (it != requests_.end()) {
     const int request_id = it->first;
     ++it;
     RejectRequest(request_id, base::File::FILE_ERROR_ABORT);
   }
+
+  DCHECK_EQ(0u, requests_.size());
 }
 
 int RequestManager::CreateRequest(const SuccessCallback& success_callback,
                                   const ErrorCallback& error_callback) {
   // The request id is unique per request manager, so per service, thereof
   // per profile.
   int request_id = next_id_++;
 
   // If cycled the int, then signal an error.
   if (requests_.find(request_id) != requests_.end())
     return 0;
 
-  Request request;
-  request.success_callback = success_callback;
-  request.error_callback = error_callback;
+  Request* request = new Request;
+  request->success_callback = success_callback;
+  request->error_callback = error_callback;
+  request->timeout_timer.Start(
+      FROM_HERE,
+      base::TimeDelta::FromMilliseconds(timeout_),
+      base::Bind(
+          &RequestManager::OnRequestTimeout,
+          base::Unretained(this),  // The timer is outlived by RequestManager.

[hashimoto, 2014/04/18 05:28:09]

Why don't you use WeakPtrFactory?

[mtomasz, 2014/04/18 05:56:18]

That would be completely redundant. Timers are owned by Request, and Requests
are owned by RequestManager. So that timer is outlived by RequestManager,
therefore this is safe.

I had a similar discussion here:
https://codereview.chromium.org/223823004/diff/100001/chrome/browser/ui/ash/m...

And I was told, that using Unretained is OK for the same reason as I said above.

[hashimoto, 2014/04/18 06:08:00]

I'd use WeakPtr because there is a period during the destruction of
RequestManager where the timer is alive, but RequestManager is partially
destructed.
As you said, however, it's also true that it shouldn't cause problems in this
current code.
I'm OK with use of Unretained.

[mtomasz, 2014/04/18 06:19:51]

Makes sense to me. Let's go with weak ptrs for simplicity. Done.

+          request_id));
   requests_[request_id] = request;
 
   return request_id;
 }
 
 bool RequestManager::FulfillRequest(int request_id,
                                     scoped_ptr<base::DictionaryValue> response,
                                     bool has_next) {
   RequestMap::iterator request_it = requests_.find(request_id);
 
   if (request_it == requests_.end())
     return false;
 
-  if (!request_it->second.success_callback.is_null())
-    request_it->second.success_callback.Run(response.Pass(), has_next);
-  if (!has_next)
+  if (!request_it->second->success_callback.is_null())
+    request_it->second->success_callback.Run(response.Pass(), has_next);
+  if (!has_next) {
+    delete request_it->second;
     requests_.erase(request_it);
+  } else {
+    request_it->second->timeout_timer.Reset();
+  }
 
   return true;
 }
 
 bool RequestManager::RejectRequest(int request_id, base::File::Error error) {
   RequestMap::iterator request_it = requests_.find(request_id);
 
   if (request_it == requests_.end())
     return false;
 
-  if (!request_it->second.error_callback.is_null())
-    request_it->second.error_callback.Run(error);
+  if (!request_it->second->error_callback.is_null())
+    request_it->second->error_callback.Run(error);
+  delete request_it->second;
   requests_.erase(request_it);
 
   return true;
 }
 
+void RequestManager::SetTimeoutForTests(int timeout) { timeout_ = timeout; }
+
+void RequestManager::OnRequestTimeout(int request_id) {
+  RejectRequest(request_id, base::File::FILE_ERROR_ABORT);
+}
+
 RequestManager::Request::Request() {}
 
 RequestManager::Request::~Request() {}
 
 }  // namespace file_system_provider
 }  // namespace chromeos