Use the ThreadPool for all isolates and native ports. Previously,

runtime/lib/isolate.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "platform/assert.h"
 #include "vm/bootstrap_natives.h"
 #include "vm/class_finalizer.h"
 #include "vm/dart.h"
 #include "vm/dart_api_impl.h"
 #include "vm/dart_entry.h"
 #include "vm/exceptions.h"
 #include "vm/longjump.h"
-#include "vm/message.h"
+#include "vm/message_handler.h"
 #include "vm/object.h"
 #include "vm/object_store.h"
 #include "vm/port.h"
 #include "vm/resolver.h"
 #include "vm/snapshot.h"
 #include "vm/thread.h"
 
 namespace dart {
 
 class IsolateStartData {
@@ skipping 22 matching lines @@
 
 static uint8_t* SerializeObject(const Instance& obj) {
   uint8_t* result = NULL;
   SnapshotWriter writer(Snapshot::kMessage, &result, &allocator);
   writer.WriteObject(obj.raw());
   writer.FinalizeBuffer();
   return result;
 }
 
 
-// TODO(turnidge): Taking down the whole vm when an isolate fails is
-// bad.  Change this.
-static void ProcessError(const Object& obj) {
+static void StoreError(Isolate* isolate, const Object& obj) {
   ASSERT(obj.IsError());
   Error& error = Error::Handle();
   error ^= obj.raw();
-  OS::PrintErr("%s\n", error.ToErrorCString());
-  exit(255);
+  isolate->object_store()->set_sticky_error(error);
 }
 
 
 static void ThrowErrorException(Exceptions::ExceptionType type,
                                 const char* error_msg,
                                 const char* library_url,
                                 const char* class_name) {
   String& str = String::Handle();
   String& name = String::Handle();
   str ^= String::New(error_msg);
@@ skipping 32 matching lines @@
   arguments.Add(&Integer::Handle(Integer::New(port_id)));
   const Object& result = Object::Handle(
       DartEntry::InvokeStatic(function, arguments, kNoArgumentNames));
   if (!result.IsError()) {
     PortMap::SetLive(port_id);
   }
   return result.raw();
 }
 
 
-static void RunIsolate(uword parameter) {
+static bool RunIsolate(uword parameter) {
   IsolateStartData* data = reinterpret_cast<IsolateStartData*>(parameter);
   Isolate* isolate = data->isolate_;
   char* library_url = data->library_url_;
   char* class_name = data->class_name_;
   intptr_t port_id = data->port_id_;
   delete data;
 
-  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));
-  LongJump* base = isolate->long_jump_base();
-  LongJump jump;
-  isolate->set_long_jump_base(&jump);
-  if (setjmp(*jump.Set()) == 0) {
+  {
+    SetIsolateScope set_scope(isolate);
     Zone zone(isolate);
     HandleScope handle_scope(isolate);
     ASSERT(ClassFinalizer::FinalizePendingClasses());
     // Lookup the target class by name, create an instance and call the run
     // method.
     const String& lib_name = String::Handle(String::NewSymbol(library_url));
+    free(library_url);
     const Library& lib = Library::Handle(Library::LookupLibrary(lib_name));
     ASSERT(!lib.IsNull());
     const String& cls_name = String::Handle(String::NewSymbol(class_name));
+    free(class_name);
     const Class& target_class = Class::Handle(lib.LookupClass(cls_name));
     // TODO(iposva): Deserialize or call the constructor after allocating.
     // For now, we only support a non-parameterized or raw target class.

[siva, 2012/04/14 00:29:53]

Is this comment still valid considering the new isolate invocation APIs

[turnidge, 2012/04/17 23:46:55]

RunIsolate implements the old isolate api.  The new isolate api uses
RunIsolate2.

     const Instance& target = Instance::Handle(Instance::New(target_class));
     Object& result = Object::Handle();
 
     // Invoke the default constructor.
     const String& period = String::Handle(String::New("."));
     String& constructor_name = String::Handle(String::Concat(cls_name, period));
     const Function& default_constructor =
         Function::Handle(target_class.LookupConstructor(constructor_name));
     if (!default_constructor.IsNull()) {
       GrowableArray<const Object*> arguments(1);
       arguments.Add(&target);
       arguments.Add(&Smi::Handle(Smi::New(Function::kCtorPhaseAll)));
       const Array& kNoArgumentNames = Array::Handle();
       result = DartEntry::InvokeStatic(default_constructor,
                                        arguments,
                                        kNoArgumentNames);
       if (result.IsError()) {
-        ProcessError(result);
+        StoreError(isolate, result);
+        return false;
       }
       ASSERT(result.IsNull());
     }
 
     // Invoke the "_run" method.
     const Function& target_function = Function::Handle(Resolver::ResolveDynamic(
         target, String::Handle(String::NewSymbol("_run")), 2, 0));
     // TODO(iposva): Proper error checking here.
     ASSERT(!target_function.IsNull());
     // TODO(iposva): Allocate the proper port number here.
     const Object& local_port = Object::Handle(ReceivePortCreate(port_id));
     if (local_port.IsError()) {
-      ProcessError(local_port);
+      StoreError(isolate, local_port);

[siva, 2012/04/14 00:29:53]

need a "return false"; here?
We don't want to invoke the target function after an error has occurred.

[turnidge, 2012/04/17 23:46:55]

Done.

     }
     GrowableArray<const Object*> arguments(1);
     arguments.Add(&local_port);
     const Array& kNoArgumentNames = Array::Handle();
     result = DartEntry::InvokeDynamic(target,
                                       target_function,
                                       arguments,
                                       kNoArgumentNames);
     if (result.IsError()) {
-      ProcessError(result);
+      StoreError(isolate, result);
+      return false;
     }
     ASSERT(result.IsNull());
-    free(class_name);
-    free(library_url);
-    result = isolate->StandardRunLoop();
-    if (result.IsError()) {
-      ProcessError(result);
-    }
-    ASSERT(result.IsNull());
-
-  } else {
-    Zone zone(isolate);
-    HandleScope handle_scope(isolate);
-    const Error& error = Error::Handle(
-        Isolate::Current()->object_store()->sticky_error());
-    const char* errmsg = error.ToErrorCString();
-    OS::PrintErr("%s\n", errmsg);
-    exit(255);
   }
-  isolate->set_long_jump_base(base);
-  Dart::ShutdownIsolate();
+  return true;

[siva, 2012/04/14 00:29:53]

In the old code if the new isolate had some errors during invocation (e.g: class
not found or script compilation error etc.) it was shutting itself down. Where
does that happen in the new scenario?

[turnidge, 2012/04/17 23:46:55]

Okay.  Thanks, I had that all wrong.  I made some major changes to the CL to
handle termination correctly.  I have dropped RunBlocking and I now pass an
end_callback to Run() so that each time we run an isolate on the thread pool we
can specify what it should do on termination.

 }
 
 
 static bool CheckArguments(const char* library_url, const char* class_name) {
   Isolate* isolate = Isolate::Current();
   Zone zone(isolate);
   HandleScope handle_scope(isolate);
   String& name = String::Handle();
   if (!ClassFinalizer::FinalizePendingClasses()) {
     return false;
@@ skipping 74 matching lines @@
     ASSERT(spawned_isolate != NULL);
     // Check arguments to see if the specified library and classes are
     // loaded, this check will throw an exception if they are not loaded.
     if (init_successful && CheckArguments(library_url, class_name)) {
       port_id = spawned_isolate->main_port();
       uword data = reinterpret_cast<uword>(
           new IsolateStartData(spawned_isolate,
                                strdup(library_url),
                                strdup(class_name),
                                port_id));
-      int result = Thread::Start(RunIsolate, data);
-      if (result != 0) {
-        FATAL1("Failed to start isolate thread %d", result);
-      }
+      Isolate::SetCurrent(NULL);
+      spawned_isolate->message_handler()->Run(
+          Dart::thread_pool(), RunIsolate, data);
     } else {
       // Error spawning the isolate, maybe due to initialization errors or
       // errors while loading the application into spawned isolate, shut
       // it down and report error.
       // Make sure to grab the error message out of the isolate before it has
       // been shutdown and to allocate it in the preserved isolates zone.
       {
         Zone zone(spawned_isolate);
         HandleScope scope(spawned_isolate);
         const Error& err_obj = Error::Handle(
@@ skipping 104 matching lines @@
         String::Handle(String::NewSymbol(function_name()));
 
     const Library& lib = Library::Handle(Library::LookupLibrary(lib_url));
     if (lib.IsNull() || lib.IsError()) {
       return Function::null();
     }
     return lib.LookupLocalFunction(func_name);
   }
 
   void Cleanup() {
-    Isolate* saved = Isolate::Current();
-    Isolate::SetCurrent(isolate());
+    SetIsolateScope set_scope(isolate());
     Dart::ShutdownIsolate();
-    Isolate::SetCurrent(saved);
   }
 
  private:
   Isolate* isolate_;
   char* library_url_;
   char* function_name_;
 };
 
 
 static bool CreateIsolate(SpawnState* state, char** error) {
@@ skipping 30 matching lines @@
     }
   }
   if (!retval) {
     Dart::ShutdownIsolate();
   }
   Isolate::SetCurrent(parent_isolate);
   return retval;
 }
 
 
-static void RunIsolate2(uword parameter) {
+static bool 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));
-
   {
+    SetIsolateScope set_scope(isolate);
     Zone zone(isolate);
     HandleScope handle_scope(isolate);
     ASSERT(ClassFinalizer::FinalizePendingClasses());
     Object& result = Object::Handle();
 
     const Function& func = Function::Handle(state->ResolveFunction());
     delete state;
     state = NULL;
     ASSERT(!func.IsNull());
 
     GrowableArray<const Object*> args(0);
     const Array& kNoArgNames = Array::Handle();
     result = DartEntry::InvokeStatic(func, args, kNoArgNames);
     if (result.IsError()) {
-      ProcessError(result);
+      StoreError(isolate, result);
+      return false;
     }
-
-    result = isolate->StandardRunLoop();
-    if (result.IsError()) {
-      ProcessError(result);
-    }
-    ASSERT(result.IsNull());
   }
-  Dart::ShutdownIsolate();
+  return true;
 }
 
 
 DEFINE_NATIVE_ENTRY(isolate_spawnFunction, 1) {
   GET_NATIVE_ARGUMENT(Closure, closure, arguments->At(0));
   const Function& func = Function::Handle(closure.function());
   const Class& cls = Class::Handle(func.owner());
   if (!func.IsClosureFunction() || !func.is_static() || !cls.IsTopLevel()) {
     const String& msg = String::Handle(String::New(
         "spawnFunction expects to be passed a closure to a top-level static "
@@ skipping 18 matching lines @@
   // Try to create a SendPort for the new isolate.
   const Object& port = Object::Handle(
       DartLibraryCalls::NewSendPort(state->isolate()->main_port()));
   if (port.IsError()) {
     state->Cleanup();
     delete state;
     Exceptions::PropagateError(port);
   }
 
   // Start the new isolate.
-  int result = Thread::Start(RunIsolate2, reinterpret_cast<uword>(state));
+  // int result = Thread::Start(RunIsolate2, reinterpret_cast<uword>(state));
+  state->isolate()->message_handler()->Run(
+      Dart::thread_pool(), RunIsolate2, reinterpret_cast<uword>(state));
+  int result = 0;
   if (result != 0) {

[siva, 2012/04/14 00:29:53]

result has just been initialized to 0, how can it be non zero, is the check
needed?

[turnidge, 2012/04/17 23:46:55]

The check is no longer needed.  It is now a FATAL in thread_pool.cc.

Do you think that is too severe?  I could restore the old behavior by adding a
return value to ThreadPool::Run()...

[siva, 2012/04/18 22:05:00]

FATAL is fine for now, we could revisit once the new code has baked for some
time.

On 2012/04/17 23:46:55, turnidge wrote:

     const String& msg = String::Handle(String::NewFormatted(
         "Failed to start thread for isolate '%s'.  Error code '%d'.",
         state->isolate()->name(), result));
     state->Cleanup();
     delete state;
     ThrowIsolateSpawnException(msg);
   }
 
   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