Issue 5183 - Provide async interface for FileSystem

src/FileSystemJsObject.cpp

Index: src/FileSystemJsObject.cpp
===================================================================
--- a/src/FileSystemJsObject.cpp
+++ b/src/FileSystemJsObject.cpp
@@ -18,320 +18,183 @@
 #include <AdblockPlus/FileSystem.h>
 #include <stdexcept>
 #include <sstream>
 #include <vector>
 
 #include <AdblockPlus/JsValue.h>
 #include "FileSystemJsObject.h"
 #include "JsContext.h"
-#include "Thread.h"
 #include "Utils.h"
 
 using namespace AdblockPlus;
 using AdblockPlus::Utils::ThrowExceptionInJS;
 
 namespace
 {
-  class IoThread : public Thread
-  {
-  public:
-    IoThread(const JsEnginePtr& jsEngine, const JsValue& callback)
-      : Thread(true), jsEngine(jsEngine), fileSystem(jsEngine->GetFileSystem()),
-        callback(callback)
-    {
-    }
-
-  protected:
-    JsEnginePtr jsEngine;
-    FileSystemPtr fileSystem;
-    JsValue callback;
-  };
-
-  class ReadThread : public IoThread
-  {
-  public:
-    ReadThread(const JsEnginePtr& jsEngine, const JsValue& callback,
-               const std::string& path)
-      : IoThread(jsEngine, callback), path(path)
-    {
-    }
-
-    void Run()
-    {
-      std::string content;
-      std::string error;
-      try
-      {
-        std::shared_ptr<std::istream> stream = fileSystem->Read(path);
-        content = Utils::Slurp(*stream);
-      }
-      catch (std::exception& e)
-      {
-        error = e.what();
-      }
-      catch (...)
-      {
-        error =  "Unknown error while reading from " + path;
-      }
-
-      const JsContext context(*jsEngine);
-      auto result = jsEngine->NewObject();
-      result.SetProperty("content", content);
-      result.SetProperty("error", error);
-      JsValueList params;
-      params.push_back(result);
-      callback.Call(params);
-    }
-
-  private:
-    std::string path;
-  };
-
-  class WriteThread : public IoThread
-  {
-  public:
-    WriteThread(const JsEnginePtr& jsEngine, const JsValue& callback,
-                const std::string& path, const std::string& content)
-      : IoThread(jsEngine, callback), path(path), content(content)
-    {
-    }
-
-    void Run()
-    {
-      std::string error;
-      try
-      {
-        std::stringstream stream;
-        stream << content;
-        fileSystem->Write(path, stream);
-      }
-      catch (std::exception& e)
-      {
-        error = e.what();
-      }
-      catch (...)
-      {
-        error = "Unknown error while writing to " + path;
-      }
-
-      const JsContext context(*jsEngine);
-      auto errorValue = jsEngine->NewValue(error);
-      JsValueList params;
-      params.push_back(errorValue);
-      callback.Call(params);
-    }
-
-  private:
-    std::string path;
-    std::string content;
-  };
-
-  class MoveThread : public IoThread
-  {
-  public:
-    MoveThread(const JsEnginePtr& jsEngine, const JsValue& callback,
-               const std::string& fromPath, const std::string& toPath)
-      : IoThread(jsEngine, callback), fromPath(fromPath), toPath(toPath)
-    {
-    }
-
-    void Run()
-    {
-      std::string error;
-      try
-      {
-        fileSystem->Move(fromPath, toPath);
-      }
-      catch (std::exception& e)
-      {
-        error = e.what();
-      }
-      catch (...)
-      {
-        error = "Unknown error while moving " + fromPath + " to " + toPath;
-      }
-
-      const JsContext context(*jsEngine);
-      auto errorValue = jsEngine->NewValue(error);
-      JsValueList params;
-      params.push_back(errorValue);
-      callback.Call(params);
-    }
-
-  private:
-    std::string fromPath;
-    std::string toPath;
-  };
-
-  class RemoveThread : public IoThread
-  {
-  public:
-    RemoveThread(const JsEnginePtr& jsEngine, const JsValue& callback,
-                 const std::string& path)
-      : IoThread(jsEngine, callback), path(path)
-    {
-    }
-
-    void Run()
-    {
-      std::string error;
-      try
-      {
-        fileSystem->Remove(path);
-      }
-      catch (std::exception& e)
-      {
-        error = e.what();
-      }
-      catch (...)
-      {
-        error = "Unknown error while removing " + path;
-      }
-
-      const JsContext context(*jsEngine);
-      auto errorValue = jsEngine->NewValue(error);
-      JsValueList params;
-      params.push_back(errorValue);
-      callback.Call(params);
-    }
-
-  private:
-    std::string path;
-  };
-
-
-  class StatThread : public IoThread
-  {
-  public:
-    StatThread(const JsEnginePtr& jsEngine, const JsValue& callback,
-               const std::string& path)
-      : IoThread(jsEngine, callback), path(path)
-    {
-    }
-
-    void Run()
-    {
-      std::string error;
-      FileSystem::StatResult statResult;
-      try
-      {
-        statResult = fileSystem->Stat(path);
-      }
-      catch (std::exception& e)
-      {
-        error = e.what();
-      }
-      catch (...)
-      {
-        error = "Unknown error while calling stat on " + path;
-      }
-
-      const JsContext context(*jsEngine);
-      auto result = jsEngine->NewObject();
-      result.SetProperty("exists", statResult.exists);
-      result.SetProperty("isFile", statResult.isFile);
-      result.SetProperty("isDirectory", statResult.isDirectory);
-      result.SetProperty("lastModified", statResult.lastModified);
-      result.SetProperty("error", error);
-
-      JsValueList params;
-      params.push_back(result);
-      callback.Call(params);
-    }
-
-  private:
-    std::string path;
-  };
-
   void ReadCallback(const v8::FunctionCallbackInfo<v8::Value>& arguments)
   {
     AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
     AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);
 
     v8::Isolate* isolate = arguments.GetIsolate();
     if (converted.size() != 2)
       return ThrowExceptionInJS(isolate, "_fileSystem.read requires 2 parameters");
     if (!converted[1].IsFunction())
       return ThrowExceptionInJS(isolate, "Second argument to _fileSystem.read must be a function");
-    ReadThread* const readThread = new ReadThread(jsEngine, converted[1],
-        converted[0].AsString());
-    readThread->Start();
+
+    JsValue callback(converted[1]);
+    std::weak_ptr<JsEngine> weakJsEngine = jsEngine;
+    jsEngine->GetAsyncFileSystem()->Read(converted[0].AsString(),
+      [weakJsEngine, callback]

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

`JsValue callback` should not be captured by value because it holds JsEngine,
instead it should be stored in JsEngine using `jsEngine->StoreJsValues` and
obscure (weak) returned ID should be used in the closure.

[hub, 2017/07/04 19:58:27]

I'll fix that here and there.

+      (std::string&& content, const std::string& error)
+      {
+        auto jsEngine = weakJsEngine.lock();
+        if (!jsEngine)
+          return;
+
+        const JsContext context(*jsEngine);
+        auto result = jsEngine->NewObject();
+        result.SetProperty("content", std::move(content));
+        result.SetProperty("error", error);
+        callback.Call(result);
+      });
   }
 
   void WriteCallback(const v8::FunctionCallbackInfo<v8::Value>& arguments)
   {
     AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
     AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);
 
     v8::Isolate* isolate = arguments.GetIsolate();
     if (converted.size() != 3)
       return ThrowExceptionInJS(isolate, "_fileSystem.write requires 3 parameters");
     if (!converted[2].IsFunction())
       return ThrowExceptionInJS(isolate, "Third argument to _fileSystem.write must be a function");
-    WriteThread* const writeThread = new WriteThread(jsEngine, converted[2],
-        converted[0].AsString(), converted[1].AsString());
-    writeThread->Start();
+
+    JsValue callback(converted[2]);
+    std::weak_ptr<JsEngine> weakJsEngine = jsEngine;
+    jsEngine->GetAsyncFileSystem()->Write(converted[0].AsString(),
+      converted[1].AsString(),
+      [weakJsEngine, callback](const std::string& error)
+      {
+        auto jsEngine = weakJsEngine.lock();
+        if (!jsEngine)
+          return;
+
+        const JsContext context(*jsEngine);
+        JsValueList params;
+        params.push_back(jsEngine->NewValue(error));
+        callback.Call(params);
+      });
   }
 
   void MoveCallback(const v8::FunctionCallbackInfo<v8::Value>& arguments)
   {
     AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
     AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);
 
     v8::Isolate* isolate = arguments.GetIsolate();
     if (converted.size() != 3)
       return ThrowExceptionInJS(isolate, "_fileSystem.move requires 3 parameters");
     if (!converted[2].IsFunction())
       return ThrowExceptionInJS(isolate, "Third argument to _fileSystem.move must be a function");
-    MoveThread* const moveThread = new MoveThread(jsEngine, converted[2],
-        converted[0].AsString(), converted[1].AsString());
-    moveThread->Start();
+
+    JsValue callback(converted[2]);
+    std::weak_ptr<JsEngine> weakJsEngine = jsEngine;
+    jsEngine->GetAsyncFileSystem()->Move(converted[0].AsString(),
+      converted[1].AsString(),
+      [weakJsEngine, callback](const std::string& error)
+      {
+        auto jsEngine = weakJsEngine.lock();
+        if (!jsEngine)
+          return;
+
+        const JsContext context(*jsEngine);
+        auto errorValue = jsEngine->NewValue(error);
+        JsValueList params;
+        params.push_back(errorValue);

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

But the problem is that the empty string will be considered as an error in JS
code and the author of JS code is likely expecting an convention similar to
nodejs. If `error` is an empty string than we simply should not add it to
`params`.

[hub, 2017/07/04 19:58:27]

This mean we are changing the semantics of the API as the test also assume the
empty string is "no error".

But I agree with error being undefined == no error, so I'll fix the tests too.

+        callback.Call(params);
+      });
   }
 
   void RemoveCallback(const v8::FunctionCallbackInfo<v8::Value>& arguments)
   {
     AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
     AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);
 
     v8::Isolate* isolate = arguments.GetIsolate();
     if (converted.size() != 2)
       return ThrowExceptionInJS(isolate, "_fileSystem.remove requires 2 parameters");
     if (!converted[1].IsFunction())
       return ThrowExceptionInJS(isolate, "Second argument to _fileSystem.remove must be a function");
-    RemoveThread* const removeThread = new RemoveThread(jsEngine, converted[1],
-        converted[0].AsString());
-    removeThread->Start();
+
+    JsValue callback(converted[1]);
+    std::weak_ptr<JsEngine> weakJsEngine = jsEngine;
+    jsEngine->GetAsyncFileSystem()->Remove(converted[0].AsString(),
+      [weakJsEngine, callback](const std::string& error)
+      {
+        auto jsEngine = weakJsEngine.lock();
+        if (!jsEngine)
+          return;
+
+        const JsContext context(*jsEngine);
+        auto errorValue = jsEngine->NewValue(error);
+        JsValueList params;
+        params.push_back(errorValue);
+        callback.Call(params);
+      });
   }
 
   void StatCallback(const v8::FunctionCallbackInfo<v8::Value>& arguments)
   {
     AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
     AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);
 
     v8::Isolate* isolate = arguments.GetIsolate();
     if (converted.size() != 2)
       return ThrowExceptionInJS(isolate, "_fileSystem.stat requires 2 parameters");
     if (!converted[1].IsFunction())
       return ThrowExceptionInJS(isolate, "Second argument to _fileSystem.stat must be a function");
-    StatThread* const statThread = new StatThread(jsEngine, converted[1],
-        converted[0].AsString());
-    statThread->Start();
+
+    JsValue callback(converted[1]);
+    std::weak_ptr<JsEngine> weakJsEngine = jsEngine;
+    jsEngine->GetAsyncFileSystem()->Stat(converted[0].AsString(),
+      [weakJsEngine, callback]
+      (const IFileSystem::StatResult& statResult, const std::string& error)
+      {
+        auto jsEngine = weakJsEngine.lock();
+        if (!jsEngine)
+          return;
+
+        const JsContext context(*jsEngine);
+        auto result = jsEngine->NewObject();
+
+        result.SetProperty("exists", statResult.exists);
+        result.SetProperty("isFile", statResult.isFile);
+        result.SetProperty("isDirectory", statResult.isDirectory);
+        result.SetProperty("lastModified", statResult.lastModified);
+        result.SetProperty("error", error);
+
+        JsValueList params;
+        params.push_back(result);
+        callback.Call(params);
+      });
   }
 
   void ResolveCallback(const v8::FunctionCallbackInfo<v8::Value>& arguments)
   {
     AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::FromArguments(arguments);
     AdblockPlus::JsValueList converted = jsEngine->ConvertArguments(arguments);
 
     v8::Isolate* isolate = arguments.GetIsolate();
     if (converted.size() != 1)
       return ThrowExceptionInJS(isolate, "_fileSystem.resolve requires 1 parameter");
 
-    std::string resolved = jsEngine->GetFileSystem()->Resolve(converted[0].AsString());
+    std::string resolved = jsEngine->GetAsyncFileSystem()->Resolve(converted[0].AsString());
     arguments.GetReturnValue().Set(Utils::ToV8String(isolate, resolved));
   }
 }
 
 
 JsValue& FileSystemJsObject::Setup(JsEngine& jsEngine, JsValue& obj)
 {
   obj.SetProperty("read", jsEngine.NewCallback(::ReadCallback));