Issue 5183 - Provide async interface for FileSystem

test/BaseJsTest.h

Index: test/BaseJsTest.h
===================================================================
--- a/test/BaseJsTest.h
+++ b/test/BaseJsTest.h
@@ -13,20 +13,56 @@
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #ifndef MOCKS_H
 #define MOCKS_H
 
+#include <thread>
+
 #include <AdblockPlus.h>
 #include <gtest/gtest.h>
 #include "../src/Thread.h"
 
+class Sync

[sergei, 2017/06/16 15:05:55]

There is a pretty similar class in src/FilterEngine.cpp. And although I'm not a
fan of it (e.g. it's totally exception unfriendly but we cannot use
std::promise/std::future for present), I would propose to move it into utils.

[hub, 2017/06/16 21:52:54]

That's where it comes from.

The problem with Utils.h is that we can't really include it in test because it
includes some v8 headers that seems to not be allowed in test.

Also I'm ok moving this somewhere where it is useful.

[sergei, 2017/07/03 09:25:53]

Acknowledged.

+{
+public:
+  Sync()
+    : set(false)
+  {
+  }
+  void Wait(int sec = 20)

[sergei, 2017/06/16 15:05:55]

For the class in utils I think it makes sense to use uint32_t in milliseconds
here and -1 for infinite waiting.

For tests either to have a separate class with a fixed default low value or
maybe a global constant. In case of separate class I prefer wrapping instead of
inheriting.

[hub, 2017/06/16 21:52:55]

At that point I'd that have Wait() to have a std::chrono::duration as an
argument. That way we get to use whatever.

Also uint32_t and -1 one as value, not pretty. I'll use
std::chrono::duration::zero() as "no timeout"

+  {
+    std::unique_lock<std::mutex> lock(mutex);
+    while (!set)
+      cv.wait_for(lock, std::chrono::seconds(sec));
+  }
+  void Set(const std::string& error_ = std::string())

[sergei, 2017/06/16 15:05:55]

unfortunately we are not using anything like m_ prefix what I personally find
very inconvenient, but here I would recommend to use `this->error = error` and
there won't need to have "_".
Anyway, it seems this error is used only in one place, do you mind to simply
define it outside that class?

[hub, 2017/06/16 21:52:55]

This is actually for -Wshadow that sadly isn't enabled. Using "this->" doesn't
fix it.

+  {
+    {
+      std::unique_lock<std::mutex> lock(mutex);
+      set = true;
+      error = error_;
+    }
+    cv.notify_all();
+  }
+  std::string GetError()
+  {
+    std::unique_lock<std::mutex> lock(mutex);
+    return error;
+  }
+private:
+  std::mutex mutex;
+  std::condition_variable cv;
+  bool set;
+  std::string error;
+};
+
 // Strictly speaking in each test there should be a special implementation of
 // an interface, which is merely referenced by a wrapper and the latter should
 // be injected into JsEngine or what ever. However the everthing a test often
 // actually needs is the access to pending tasks. Therefore instantiation of
 // implemenation of an interface, creation of shared tasks and sharing them
 // with tasks in test is located in this class.
 //
 // Task is passed as an additional template parameter instead of using traits
@@ -97,96 +133,168 @@
 public:
   void operator()(LogLevel logLevel, const std::string& message,
         const std::string& source)
   {
     throw std::runtime_error("Unexpected error: " + message);
   }
 };
 
-class ThrowingFileSystem : public AdblockPlus::FileSystem
+class ThrowingFileSystem : public AdblockPlus::IFileSystem, public AdblockPlus::FileSystem
 {
 public:
   std::shared_ptr<std::istream> Read(const std::string& path) const
   {
     throw std::runtime_error("Not implemented");
   }
+  void Read(const std::string& path,
+            const ReadCallback& callback) const
+  {
+    throw std::runtime_error("Not implemented");
+  }
 
   void Write(const std::string& path, std::istream& content)
   {
     throw std::runtime_error("Not implemented");
   }
+  void Write(const std::string& path, std::shared_ptr<std::istream> data,
+             const Callback& callback)
+  {
+    throw std::runtime_error("Not implemented");
+  }
 
   void Move(const std::string& fromPath, const std::string& toPath)
   {
     throw std::runtime_error("Not implemented");
   }
+  void Move(const std::string& fromPath, const std::string& toPath,
+            const Callback& callback)
+  {
+    throw std::runtime_error("Not implemented");
+  }
 
   void Remove(const std::string& path)
   {
     throw std::runtime_error("Not implemented");
   }
+  void Remove(const std::string& path, const Callback& callback)
+  {
+    throw std::runtime_error("Not implemented");
+  }
 
   StatResult Stat(const std::string& path) const
   {
     throw std::runtime_error("Not implemented");
   }
+  void Stat(const std::string& path,
+            const StatCallback& callback) const
+  {
+    throw std::runtime_error("Not implemented");
+  }
 
   std::string Resolve(const std::string& path) const
   {
     throw std::runtime_error("Not implemented");
   }
-
 };
 
 class ThrowingWebRequest : public AdblockPlus::IWebRequest
 {
 public:
   void GET(const std::string& url, const AdblockPlus::HeaderList& requestHeaders, const GetCallback&) override
   {
     throw std::runtime_error("Unexpected GET: " + url);
   }
 };
 
-class LazyFileSystem : public AdblockPlus::FileSystem
+class LazyFileSystem : public AdblockPlus::IFileSystem, public AdblockPlus::FileSystem
 {
 public:
   std::shared_ptr<std::istream> Read(const std::string& path) const
   {
     std::string dummyData("");
     if (path == "patterns.ini")
       dummyData = "# Adblock Plus preferences\n[Subscription]\nurl=~fl~";
     else if (path == "prefs.json")
       dummyData = "{}";
     return std::shared_ptr<std::istream>(new std::istringstream(dummyData));
   }
 
+  void Read(const std::string& path, const ReadCallback& callback) const
+  {
+    std::thread([this, path, callback]

[sergei, 2017/06/16 15:05:55]

I actually planned to gradually convert to a special Noop implementation and not
introduce threads, that's because I said that tests can be changed later. I was
also afraid that adding threads here can introduce data race in some tests (one
need to carefully go though all of them), though it seems not the case.

OK, let's leave it as is.

[hub, 2017/06/16 21:52:55]

Acknowledged.

+    {
+      auto result = Read(path);
+      std::stringstream stream;
+      stream << result->rdbuf();
+      std::string content = stream.str();
+      callback(std::move(content), "");
+    }).detach();
+  }
+
   void Write(const std::string& path, std::istream& content)
   {
   }
 
+  void Write(const std::string& path, std::shared_ptr<std::istream> data,
+             const Callback& callback)
+  {
+    std::thread([this, path, data, callback]
+    {
+      Write(path, *data);
+      callback("");
+    }).detach();
+  }
+
   void Move(const std::string& fromPath, const std::string& toPath)
   {
   }
 
+  void Move(const std::string& fromPath, const std::string& toPath,
+            const Callback& callback)
+  {
+    std::thread([this, fromPath, toPath, callback]
+    {
+      Move(fromPath, toPath);
+      callback("");
+    }).detach();
+  }
+
   void Remove(const std::string& path)
   {
   }
 
+  void Remove(const std::string& path, const Callback& callback)
+  {
+    std::thread([this, path, callback]
+    {
+      Remove(path);
+      callback("");
+    }).detach();
+  }
+
   StatResult Stat(const std::string& path) const
   {
     StatResult result;
     if (path == "patterns.ini")
     {
       result.exists = true;
       result.isFile = true;
     }
     return result;
   }
 
+  void Stat(const std::string& path, const StatCallback& callback) const
+  {
+    std::thread([this, path, callback]
+    {
+      callback(Stat(path), "");
+    }).detach();
+  }
+
   std::string Resolve(const std::string& path) const
   {
     return path;
   }
 };
 
 class NoopWebRequest : public AdblockPlus::IWebRequest
 {