[fsp] Initial implementation of timeout logic for requests.

chrome/browser/chromeos/file_system_provider/request_manager.cc

Index: chrome/browser/chromeos/file_system_provider/request_manager.cc
diff --git a/chrome/browser/chromeos/file_system_provider/request_manager.cc b/chrome/browser/chromeos/file_system_provider/request_manager.cc
index d8aa9bb14d4a07304459f5a32b913aeee3bc4fac..cfa6dc3fef1307d5403933bea138888f29999a30 100644
--- a/chrome/browser/chromeos/file_system_provider/request_manager.cc
+++ b/chrome/browser/chromeos/file_system_provider/request_manager.cc
@@ -4,12 +4,21 @@
 
 #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.
@@ -19,6 +28,8 @@ RequestManager::~RequestManager() {
     ++it;
     RejectRequest(request_id, base::File::FILE_ERROR_ABORT);
   }
+
+  DCHECK_EQ(0u, requests_.size());
 }
 
 int RequestManager::CreateRequest(const SuccessCallback& success_callback,
@@ -31,9 +42,16 @@ int RequestManager::CreateRequest(const SuccessCallback& success_callback,
   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;
@@ -47,10 +65,14 @@ bool RequestManager::FulfillRequest(int 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;
 }
@@ -61,13 +83,20 @@ bool RequestManager::RejectRequest(int request_id, base::File::Error error) {
   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() {}