Implement spawnFunction from the new isolate api.

runtime/lib/isolate.cc

Index: runtime/lib/isolate.cc
===================================================================
--- runtime/lib/isolate.cc	(revision 5192)
+++ runtime/lib/isolate.cc	(working copy)
@@ -53,6 +53,8 @@
 }
 
 
+// TODO(turnidge): Taking down the whole vm when an isolate fails is
+// bad.  Change this.
 static void ProcessError(const Object& obj) {
   ASSERT(obj.IsError());
   Error& error = Error::Handle();
@@ -383,4 +385,163 @@
       send_id, reply_id, data, Message::kNormalPriority));
 }
 
+
+static void ThrowIllegalArgException(const char* message) {
+  const String& message_str = String::Handle(String::New(message));
+  GrowableArray<const Object*> args(1);
+  args.Add(&message_str);
+  Exceptions::ThrowByType(Exceptions::kIllegalArgument, args);
+}
+
+
+class SpawnState {
+ public:
+  explicit SpawnState(const Function& func)
+      : isolate_(NULL),
+        library_url_(NULL),
+        function_name_(NULL) {
+    const Class& cls = Class::Handle(func.owner());
+    const Library& lib = Library::Handle(cls.library());

[siva, 2012/03/12 23:25:41]

Assert cls represents implicit top level in 'lib'

[turnidge, 2012/03/13 18:13:48]

Added an assert that cls.IsTopLevel().

+    const String& lib_url = String::Handle(lib.url());
+    library_url_ = strdup(lib_url.ToCString());
+
+    const String& func_name = String::Handle(func.name());
+    function_name_ = strdup(func_name.ToCString());
+  }
+
+  ~SpawnState() {
+    free(library_url_);
+    free(function_name_);
+  }
+
+  Isolate* isolate() const { return isolate_; }
+  void set_isolate(Isolate* value) { isolate_ = value; }
+  char* library_url() const { return library_url_; }
+  char* function_name() const { return function_name_; }
+
+ private:
+  Isolate* isolate_;
+  char* library_url_;
+  char* function_name_;
+};
+
+
+static bool CreateIsolate(SpawnState* state, char** error) {
+  Isolate* parent_isolate = Isolate::Current();
+
+  Dart_IsolateCreateCallback callback = Isolate::CreateCallback();
+  ASSERT(callback);
+  const char* isolate_name = BuildIsolateName(state->library_url(),
+                                              "",
+                                              state->function_name());
+  void* init_data = parent_isolate->init_callback_data();
+  bool retval = (callback)(isolate_name, init_data, error);
+  if (retval) {
+    Isolate* child_isolate = Isolate::Current();
+    ASSERT(child_isolate);
+    state->set_isolate(child_isolate);

[siva, 2012/03/12 23:25:41]

We are assuming that the callback will load up the right library, It might be a
good idea to do a lookup of the library and function to ascertain that they
exist in the child isolate before declaring that the isolate was successfully
created.

[turnidge, 2012/03/13 18:13:48]

It seems to me that if this occurs, it is an internal error, right?  Is there
any wa that this will occur under normal circumstances?  I have added an
assertion that the function always resolves in RunIsolate2.  Is that sufficient?

[siva, 2012/03/13 19:45:21]

Since the callback is something that the embedder registers we are executing
code outside the VM (external code) and that is why I feel we should treat it as
an error and not an internal error. Kind of similar to the checks we have when
somebody invokes a dart API method.


On 2012/03/13 18:13:48, turnidge wrote:

+  } else {
+    Dart::ShutdownIsolate();
+  }
+  Isolate::SetCurrent(parent_isolate);
+  return retval;
+}
+
+
+static void RunIsolate2(uword parameter) {
+  SpawnState* state = reinterpret_cast<SpawnState*>(parameter);
+  Isolate* isolate = state->isolate();
+
+  Isolate::SetCurrent(isolate);
+  // Intialize stack limit in case we are running isolate in a
+  // different thread than in which it was initialized.
+  isolate->SetStackLimitFromCurrentTOS(reinterpret_cast<uword>(&isolate));
+
+  {
+    Zone zone(isolate);
+    HandleScope handle_scope(isolate);
+    ASSERT(ClassFinalizer::FinalizePendingClasses());
+    Object& result = Object::Handle();
+
+    const String& lib_url =
+        String::Handle(String::NewSymbol(state->library_url()));
+    const String& func_name =
+        String::Handle(String::NewSymbol(state->function_name()));
+    delete state;
+    state = NULL;
+
+    const Library& lib = Library::Handle(Library::LookupLibrary(lib_url));
+    ASSERT(!lib.IsNull() && !lib.IsError());
+    GrowableArray<const Object*> args(0);
+    const Function& func =
+        Function::Handle(lib.LookupLocalFunction(func_name));

[siva, 2012/03/12 23:25:41]

ASSERT(!func.IsNull());

[turnidge, 2012/03/13 18:13:48]

Done.

+    const Array& kNoArgNames = Array::Handle();
+    result = DartEntry::InvokeStatic(func, args, kNoArgNames);
+    if (result.IsError()) {
+      ProcessError(result);
+    }
+
+    result = isolate->StandardRunLoop();
+    if (result.IsError()) {
+      ProcessError(result);
+    }
+    ASSERT(result.IsNull());
+  }
+  Dart::ShutdownIsolate();
+}
+
+
+DEFINE_NATIVE_ENTRY(isolate_spawnFunction, 1) {
+  const Closure& closure = Closure::CheckedHandle(arguments->At(0));

[siva, 2012/03/12 23:25:41]

This should be
GET_NATIVE_ARGUMENT(Closure, closure, arguments->At(0));

so that an illegal argument exception is thrown when a non closure object is
passed in instead of calling FATAL.

[turnidge, 2012/03/13 18:13:48]

Done.

+  const Function& func = Function::Handle(closure.function());
+  const Context& ctx = Context::Handle(closure.context());
+
+  if (!func.IsClosureFunction() || !func.is_static()) {
+    ThrowIllegalArgException(
+        "spawnFunction expects to be passed a closure to a static function");
+  }

[siva, 2012/03/12 23:25:41]

Don't we also have to check that it is a top level static function?

[turnidge, 2012/03/13 18:13:48]

Done.

+  if (ctx.num_variables() > 0) {

[Siggi Cherem (dart-lang), 2012/03/12 22:57:44]

is this check redundant with the check above for is_static?

[turnidge, 2012/03/13 18:13:48]

I believe it is.  Made it an ASSERT.

+    ThrowIllegalArgException(
+        "spawnFunction expects to be passed a closure with no bound variables");
+  }
+
+  // Create a new isolate.
+  char* error = NULL;
+  SpawnState* state = new SpawnState(func);
+  if (!CreateIsolate(state, &error)) {
+    // TODO(turnidge): Throw a different exception type here.

[turnidge, 2012/03/12 22:38:48]

Siggi -- what type of exception do we want to throw here?

[Siggi Cherem (dart-lang), 2012/03/12 22:57:44]

How about we introduce 'IsolateSpawnException' (making this part of
dart:isolate)? Other ideas?

[turnidge, 2012/03/13 18:13:48]

I've added IsolateSpawnException as part of this change.

+    delete state;
+    ThrowIllegalArgException(error);
+  }
+
+  // Try to create a SendPort for the new isolate.
+  const Object& port =
+      Object::Handle(SendPortCreate(state->isolate()->main_port()));
+  if (port.IsError()) {
+    Isolate* saved = Isolate::Current();
+    Isolate::SetCurrent(state->isolate());
+    Dart::ShutdownIsolate();
+    Isolate::SetCurrent(saved);
+    delete state;
+    Exceptions::PropagateError(port);
+  }
+
+  // Start the new isolate.
+  int result = Thread::Start(RunIsolate2, reinterpret_cast<uword>(state));
+  if (result != 0) {
+    FATAL1("Failed to start isolate thread: error %d", result);

[Siggi Cherem (dart-lang), 2012/03/12 22:57:44]

should this be fatal or also throw the IsolateSpawnException?

[siva, 2012/03/12 23:25:41]

Why does this have to be a Fatal and not an exception?

[turnidge, 2012/03/13 18:13:48]

I was just cribbing from the existing code.  Changed it to an exception.

+  }
+
+  arguments->SetReturn(port);
+}
+
+
+DEFINE_NATIVE_ENTRY(isolate_getPortInternal, 0) {
+  const Object& port = Object::Handle(ReceivePortCreate(isolate->main_port()));
+  if (port.IsError()) {
+    Exceptions::PropagateError(port);
+  }
+  arguments->SetReturn(port);
+}
+
 }  // namespace dart