Issue 5183 - Provide async interface for FileSystem

test/BaseJsTest.h

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * 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
 // (CRTP does not work with types in derived class) merely to simplify the code
 // by minimization.
@@ skipping 63 matching lines @@
   void operator()(LogLevel logLevel, const std::string& message,
         const std::string& source)
   {
     throw std::runtime_error("Unexpected error: " + message);
   }
 };
 
 class ThrowingFileSystem : public AdblockPlus::IFileSystem, public AdblockPlus::FileSystem
 {
 public:
-  std::shared_ptr<std::istream> Read(const std::string& path) const
+  IOBuffer 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,
+  void Write(const std::string& path, const IOBuffer& content)
+  {
+    throw std::runtime_error("Not implemented");
+  }
+  void Write(const std::string& path, const IOBuffer& 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,
@@ skipping 32 matching lines @@
 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::IFileSystem, public AdblockPlus::FileSystem
 {
 public:
-  std::shared_ptr<std::istream> Read(const std::string& path) const
+  IOBuffer 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));
+    return IOBuffer(dummyData.cbegin(), dummyData.cend());
   }
 
   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,
+      auto data = Read(path);
+      callback(std::move(data), "");
+    }).detach();
+  }
+
+  void Write(const std::string& path, const IOBuffer& content)
+  {
+  }
+
+  void Write(const std::string& path, const IOBuffer& data,
              const Callback& callback)
   {
     std::thread([this, path, data, callback]
     {
-      Write(path, *data);
+      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)
@@ skipping 95 matching lines @@
 protected:
   AdblockPlus::JsEnginePtr jsEngine;
 
   virtual void SetUp()
   {
     jsEngine = CreateJsEngine();
   }
 };
 
 #endif