diff --git a/runtime/bin/dartutils.cc b/runtime/bin/dartutils.cc index 8e59a77fd4e..dd63bec000d 100644 --- a/runtime/bin/dartutils.cc +++ b/runtime/bin/dartutils.cc @@ -21,15 +21,6 @@ #include "bin/socket.h" #include "bin/utils.h" -// Return the error from the containing function if handle is in error handle. -#define RETURN_IF_ERROR(handle) \ - { \ - Dart_Handle __handle = handle; \ - if (Dart_IsError((__handle))) { \ - return __handle; \ - } \ - } - namespace dart { namespace bin { @@ -392,7 +383,7 @@ Dart_Handle DartUtils::LibraryTagHandler(Dart_LibraryTag tag, // Resolve the url within the context of the library's URL. Dart_Handle builtin_lib = Builtin::LoadAndCheckLibrary(Builtin::kBuiltinLibrary); - RETURN_IF_ERROR(builtin_lib); + DART_CHECK_VALID(builtin_lib); return ResolveUri(library_url, url, builtin_lib); } @@ -432,7 +423,7 @@ Dart_Handle DartUtils::LibraryTagHandler(Dart_LibraryTag tag, Dart_Handle builtin_lib = Builtin::LoadAndCheckLibrary(Builtin::kBuiltinLibrary); - RETURN_IF_ERROR(builtin_lib); + DART_CHECK_VALID(builtin_lib); if (DartUtils::IsDartExtensionSchemeURL(url_string)) { // Load a native code shared library to use in a native extension if (tag != Dart_kImportTag) { @@ -578,9 +569,7 @@ void FUNCTION_NAME(Builtin_LoadSource)(Dart_NativeArguments args) { } else { ASSERT(tag == Dart_kSourceTag); Dart_Handle library = Dart_LookupLibrary(library_uri); - if (Dart_IsError(library)) { - Dart_PropagateError(library); - } + DART_CHECK_VALID(library); result = Dart_LoadSource(library, resolved_script_uri, source, 0, 0); } } @@ -646,24 +635,24 @@ Dart_Handle DartUtils::PrepareBuiltinLibrary(Dart_Handle builtin_lib, // Setup the internal library's 'internalPrint' function. Dart_Handle print = Dart_Invoke( builtin_lib, NewString("_getPrintClosure"), 0, NULL); - RETURN_IF_ERROR(print); + DART_CHECK_VALID(print); Dart_Handle result = Dart_SetField(internal_lib, NewString("_printClosure"), print); - RETURN_IF_ERROR(result); + DART_CHECK_VALID(result); if (!is_service_isolate) { if (IsWindowsHost()) { result = Dart_SetField(builtin_lib, NewString("_isWindows"), Dart_True()); - RETURN_IF_ERROR(result); + DART_CHECK_VALID(result); } if (trace_loading) { result = Dart_SetField(builtin_lib, NewString("_traceLoading"), Dart_True()); - RETURN_IF_ERROR(result); + DART_CHECK_VALID(result); } // Set current working directory. result = SetWorkingDirectory(builtin_lib); - RETURN_IF_ERROR(result); + DART_CHECK_VALID(result); // Wait for the service isolate to initialize the load port. Dart_Port load_port = Dart_ServiceWaitForLoadPort(); if (load_port == ILLEGAL_PORT) { @@ -676,7 +665,7 @@ Dart_Handle DartUtils::PrepareBuiltinLibrary(Dart_Handle builtin_lib, if (package_root != NULL) { ASSERT(packages_file == NULL); result = NewString(package_root); - RETURN_IF_ERROR(result); + DART_CHECK_VALID(result); const int kNumArgs = 1; Dart_Handle dart_args[kNumArgs]; dart_args[0] = result; @@ -684,10 +673,10 @@ Dart_Handle DartUtils::PrepareBuiltinLibrary(Dart_Handle builtin_lib, NewString("_setPackageRoot"), kNumArgs, dart_args); - RETURN_IF_ERROR(result); + DART_CHECK_VALID(result); } else if (packages_file != NULL) { result = NewString(packages_file); - RETURN_IF_ERROR(result); + DART_CHECK_VALID(result); const int kNumArgs = 1; Dart_Handle dart_args[kNumArgs]; dart_args[0] = result; @@ -695,48 +684,47 @@ Dart_Handle DartUtils::PrepareBuiltinLibrary(Dart_Handle builtin_lib, NewString("_loadPackagesMap"), kNumArgs, dart_args); - RETURN_IF_ERROR(result); + DART_CHECK_VALID(result); } return Dart_True(); } -Dart_Handle DartUtils::PrepareCoreLibrary(Dart_Handle core_lib, - Dart_Handle builtin_lib, - bool is_service_isolate) { +void DartUtils::PrepareCoreLibrary(Dart_Handle core_lib, + Dart_Handle builtin_lib, + bool is_service_isolate) { if (!is_service_isolate) { // Setup the 'Uri.base' getter in dart:core. Dart_Handle uri_base = Dart_Invoke( builtin_lib, NewString("_getUriBaseClosure"), 0, NULL); - RETURN_IF_ERROR(uri_base); + DART_CHECK_VALID(uri_base); Dart_Handle result = Dart_SetField(core_lib, NewString("_uriBaseClosure"), uri_base); - RETURN_IF_ERROR(result); + DART_CHECK_VALID(result); } - return Dart_True(); } -Dart_Handle DartUtils::PrepareAsyncLibrary(Dart_Handle async_lib, - Dart_Handle isolate_lib) { +void DartUtils::PrepareAsyncLibrary(Dart_Handle async_lib, + Dart_Handle isolate_lib) { Dart_Handle schedule_immediate_closure = Dart_Invoke(isolate_lib, NewString("_getIsolateScheduleImmediateClosure"), 0, NULL); Dart_Handle args[1]; args[0] = schedule_immediate_closure; - return Dart_Invoke( - async_lib, NewString("_setScheduleImmediateClosure"), 1, args); + DART_CHECK_VALID(Dart_Invoke( + async_lib, NewString("_setScheduleImmediateClosure"), 1, args)); } -Dart_Handle DartUtils::PrepareIOLibrary(Dart_Handle io_lib) { - return Dart_Invoke(io_lib, NewString("_setupHooks"), 0, NULL); +void DartUtils::PrepareIOLibrary(Dart_Handle io_lib) { + DART_CHECK_VALID(Dart_Invoke(io_lib, NewString("_setupHooks"), 0, NULL)); } -Dart_Handle DartUtils::PrepareIsolateLibrary(Dart_Handle isolate_lib) { - return Dart_Invoke(isolate_lib, NewString("_setupHooks"), 0, NULL); +void DartUtils::PrepareIsolateLibrary(Dart_Handle isolate_lib) { + DART_CHECK_VALID(Dart_Invoke(isolate_lib, NewString("_setupHooks"), 0, NULL)); } @@ -747,28 +735,28 @@ Dart_Handle DartUtils::PrepareForScriptLoading(const char* package_root, Dart_Handle builtin_lib) { // First ensure all required libraries are available. Dart_Handle url = NewString(kCoreLibURL); - RETURN_IF_ERROR(url); + DART_CHECK_VALID(url); Dart_Handle core_lib = Dart_LookupLibrary(url); - RETURN_IF_ERROR(core_lib); + DART_CHECK_VALID(core_lib); url = NewString(kAsyncLibURL); - RETURN_IF_ERROR(url); + DART_CHECK_VALID(url); Dart_Handle async_lib = Dart_LookupLibrary(url); - RETURN_IF_ERROR(async_lib); + DART_CHECK_VALID(async_lib); url = NewString(kIsolateLibURL); - RETURN_IF_ERROR(url); + DART_CHECK_VALID(url); Dart_Handle isolate_lib = Dart_LookupLibrary(url); - RETURN_IF_ERROR(isolate_lib); + DART_CHECK_VALID(isolate_lib); url = NewString(kInternalLibURL); - RETURN_IF_ERROR(url); + DART_CHECK_VALID(url); Dart_Handle internal_lib = Dart_LookupLibrary(url); - RETURN_IF_ERROR(internal_lib); + DART_CHECK_VALID(internal_lib); Dart_Handle io_lib = Builtin::LoadAndCheckLibrary(Builtin::kIOLibrary); - RETURN_IF_ERROR(io_lib); + DART_CHECK_VALID(io_lib); // We need to ensure that all the scripts loaded so far are finalized // as we are about to invoke some Dart code below to setup closures. Dart_Handle result = Dart_FinalizeLoading(false); - RETURN_IF_ERROR(result); + DART_CHECK_VALID(result); result = PrepareBuiltinLibrary(builtin_lib, internal_lib, @@ -776,32 +764,30 @@ Dart_Handle DartUtils::PrepareForScriptLoading(const char* package_root, trace_loading, package_root, packages_file); - RETURN_IF_ERROR(result); + DART_CHECK_VALID(result); - RETURN_IF_ERROR(PrepareAsyncLibrary(async_lib, isolate_lib)); - RETURN_IF_ERROR(PrepareCoreLibrary( - core_lib, builtin_lib, is_service_isolate)); - RETURN_IF_ERROR(PrepareIsolateLibrary(isolate_lib)); - RETURN_IF_ERROR(PrepareIOLibrary(io_lib)); + PrepareAsyncLibrary(async_lib, isolate_lib); + PrepareCoreLibrary(core_lib, builtin_lib, is_service_isolate); + PrepareIsolateLibrary(isolate_lib); + PrepareIOLibrary(io_lib); return result; } -Dart_Handle DartUtils::SetupIOLibrary(const char* script_uri) { +void DartUtils::SetupIOLibrary(const char* script_uri) { Dart_Handle io_lib_url = NewString(kIOLibURL); - RETURN_IF_ERROR(io_lib_url); + DART_CHECK_VALID(io_lib_url); Dart_Handle io_lib = Dart_LookupLibrary(io_lib_url); - RETURN_IF_ERROR(io_lib); + DART_CHECK_VALID(io_lib); Dart_Handle platform_type = GetDartType(DartUtils::kIOLibURL, "_Platform"); - RETURN_IF_ERROR(platform_type); + DART_CHECK_VALID(platform_type); Dart_Handle script_name = NewString("_nativeScript"); - RETURN_IF_ERROR(script_name); + DART_CHECK_VALID(script_name); Dart_Handle dart_script = NewString(script_uri); - RETURN_IF_ERROR(dart_script); + DART_CHECK_VALID(dart_script); Dart_Handle set_script_name = Dart_SetField(platform_type, script_name, dart_script); - RETURN_IF_ERROR(set_script_name); - return Dart_Null(); + DART_CHECK_VALID(set_script_name); } diff --git a/runtime/bin/dartutils.h b/runtime/bin/dartutils.h index 83626e0b79d..8a9c9530331 100644 --- a/runtime/bin/dartutils.h +++ b/runtime/bin/dartutils.h @@ -130,19 +130,19 @@ class DartUtils { bool trace_loading, const char* package_root, const char* packages_file); - static Dart_Handle PrepareCoreLibrary(Dart_Handle core_lib, + static void PrepareCoreLibrary(Dart_Handle core_lib, Dart_Handle builtin_lib, bool is_service_isolate); - static Dart_Handle PrepareAsyncLibrary(Dart_Handle async_lib, + static void PrepareAsyncLibrary(Dart_Handle async_lib, Dart_Handle isolate_lib); - static Dart_Handle PrepareIOLibrary(Dart_Handle io_lib); - static Dart_Handle PrepareIsolateLibrary(Dart_Handle isolate_lib); + static void PrepareIOLibrary(Dart_Handle io_lib); + static void PrepareIsolateLibrary(Dart_Handle isolate_lib); static Dart_Handle PrepareForScriptLoading(const char* package_root, const char* packages_file, bool is_service_isolate, bool trace_loading, Dart_Handle builtin_lib); - static Dart_Handle SetupIOLibrary(const char* script_uri); + static void SetupIOLibrary(const char* script_uri); static bool PostNull(Dart_Port port_id); static bool PostInt32(Dart_Port port_id, int32_t value); diff --git a/runtime/bin/dbg_message.cc b/runtime/bin/dbg_message.cc index 0a3038a619d..a6358806dac 100644 --- a/runtime/bin/dbg_message.cc +++ b/runtime/bin/dbg_message.cc @@ -1402,17 +1402,14 @@ void DbgMsgQueueList::ExceptionThrownHandler(Dart_IsolateId isolate_id, void DbgMsgQueueList::IsolateEventHandler(Dart_IsolateId isolate_id, Dart_IsolateEvent kind) { + DebuggerConnectionHandler::WaitForConnection(); + Dart_EnterScope(); if (kind == kCreated) { - DebuggerConnectionHandler::WaitForConnection(); - Dart_EnterScope(); DbgMsgQueue* msg_queue = AddIsolateMsgQueue(isolate_id); msg_queue->SendIsolateEvent(isolate_id, kind); - Dart_ExitScope(); } else { DbgMsgQueue* msg_queue = GetIsolateMsgQueue(isolate_id); if (msg_queue != NULL) { - DebuggerConnectionHandler::WaitForConnection(); - Dart_EnterScope(); msg_queue->SendQueuedMsgs(); msg_queue->SendIsolateEvent(isolate_id, kind); if (kind == kInterrupted) { @@ -1421,11 +1418,9 @@ void DbgMsgQueueList::IsolateEventHandler(Dart_IsolateId isolate_id, ASSERT(kind == kShutdown); RemoveIsolateMsgQueue(isolate_id); } - Dart_ExitScope(); } - // If there is no receive message queue, do not wait for a connection, and - // ignore the message. } + Dart_ExitScope(); } } // namespace bin diff --git a/runtime/bin/gen_snapshot.cc b/runtime/bin/gen_snapshot.cc index 82bf5ddec1f..2f19fb23f8f 100644 --- a/runtime/bin/gen_snapshot.cc +++ b/runtime/bin/gen_snapshot.cc @@ -544,18 +544,18 @@ int main(int argc, char** argv) { // Initialize the Dart VM. // Note: We don't expect isolates to be created from dart code during // snapshot generation. - char* error = Dart_Initialize(NULL, NULL, NULL, NULL, NULL, - DartUtils::OpenFile, - DartUtils::ReadFile, - DartUtils::WriteFile, - DartUtils::CloseFile, - DartUtils::EntropySource); - if (error != NULL) { - Log::PrintErr("VM initialization failed: %s\n", error); - free(error); + if (!Dart_Initialize(NULL, + NULL, NULL, NULL, NULL, + DartUtils::OpenFile, + DartUtils::ReadFile, + DartUtils::WriteFile, + DartUtils::CloseFile, + DartUtils::EntropySource)) { + Log::PrintErr("VM initialization failed\n"); return 255; } + char* error; Dart_Isolate isolate = Dart_CreateIsolate( NULL, NULL, NULL, NULL, NULL, &error); if (isolate == NULL) { diff --git a/runtime/bin/main.cc b/runtime/bin/main.cc index 7efcdb1d634..5c36f41abee 100644 --- a/runtime/bin/main.cc +++ b/runtime/bin/main.cc @@ -108,14 +108,7 @@ static void ErrorExit(int exit_code, const char* format, ...) { Dart_ExitScope(); Dart_ShutdownIsolate(); - // Terminate process exit-code handler. - Process::TerminateExitCodeHandler(); - - char* error = Dart_Cleanup(); - if (error != NULL) { - Log::PrintErr("VM cleanup failed: %s\n", error); - free(error); - } + Dart_Cleanup(); exit(exit_code); } @@ -631,7 +624,6 @@ static Dart_Isolate CreateIsolateAndSetupHelper(const char* script_uri, error); if (isolate == NULL) { - delete isolate_data; return NULL; } @@ -696,8 +688,7 @@ static Dart_Isolate CreateIsolateAndSetupHelper(const char* script_uri, Platform::SetPackageRoot(package_root); - result = DartUtils::SetupIOLibrary(script_uri); - CHECK_RESULT(result); + DartUtils::SetupIOLibrary(script_uri); // Make the isolate runnable so that it is ready to handle messages. Dart_ExitScope(); @@ -1038,17 +1029,15 @@ void main(int argc, char** argv) { } // Initialize the Dart VM. - char* error = Dart_Initialize(vm_isolate_snapshot_buffer, - CreateIsolateAndSetup, NULL, NULL, ShutdownIsolate, - DartUtils::OpenFile, - DartUtils::ReadFile, - DartUtils::WriteFile, - DartUtils::CloseFile, - DartUtils::EntropySource); - if (error != NULL) { - fprintf(stderr, "VM initialization failed: %s\n", error); + if (!Dart_Initialize(vm_isolate_snapshot_buffer, + CreateIsolateAndSetup, NULL, NULL, ShutdownIsolate, + DartUtils::OpenFile, + DartUtils::ReadFile, + DartUtils::WriteFile, + DartUtils::CloseFile, + DartUtils::EntropySource)) { + fprintf(stderr, "%s", "VM initialization failed\n"); fflush(stderr); - free(error); exit(kErrorExitCode); } @@ -1059,6 +1048,7 @@ void main(int argc, char** argv) { // Call CreateIsolateAndSetup which creates an isolate and loads up // the specified application script. + char* error = NULL; int exit_code = 0; char* isolate_name = BuildIsolateName(script_name, "main"); Dart_Isolate isolate = CreateIsolateAndSetupHelper(script_name, @@ -1071,14 +1061,7 @@ void main(int argc, char** argv) { if (isolate == NULL) { Log::PrintErr("%s\n", error); free(error); - error = NULL; delete [] isolate_name; - Process::TerminateExitCodeHandler(); - error = Dart_Cleanup(); - if (error != NULL) { - Log::PrintErr("VM cleanup failed: %s\n", error); - free(error); - } exit((exit_code != 0) ? exit_code : kErrorExitCode); } delete [] isolate_name; @@ -1182,11 +1165,7 @@ void main(int argc, char** argv) { // Terminate process exit-code handler. Process::TerminateExitCodeHandler(); - error = Dart_Cleanup(); - if (error != NULL) { - Log::PrintErr("VM cleanup failed: %s\n", error); - free(error); - } + Dart_Cleanup(); // Free copied argument strings if converted. if (argv_converted) { diff --git a/runtime/bin/process.cc b/runtime/bin/process.cc index c678dfc4c8b..2c90c11cdbe 100644 --- a/runtime/bin/process.cc +++ b/runtime/bin/process.cc @@ -4,7 +4,6 @@ #include "bin/dartutils.h" #include "bin/io_buffer.h" -#include "bin/log.h" #include "bin/platform.h" #include "bin/process.h" #include "bin/socket.h" @@ -217,13 +216,8 @@ void FUNCTION_NAME(Process_Exit)(Dart_NativeArguments args) { int64_t status = 0; // Ignore result if passing invalid argument and just exit 0. DartUtils::GetInt64Value(Dart_GetNativeArgument(args, 0), &status); - Dart_ShutdownIsolate(); - Process::TerminateExitCodeHandler(); - char* error = Dart_Cleanup(); - if (error != NULL) { - Log::PrintErr("VM cleanup failed: %s\n", error); - free(error); - } + Dart_ExitIsolate(); + Dart_Cleanup(); exit(static_cast(status)); } diff --git a/runtime/bin/run_vm_tests.cc b/runtime/bin/run_vm_tests.cc index cb5899ec3f9..388006256ea 100644 --- a/runtime/bin/run_vm_tests.cc +++ b/runtime/bin/run_vm_tests.cc @@ -113,10 +113,6 @@ static int Main(int argc, const char** argv) { TestCaseBase::RunAll(); // Apply the filter to all registered benchmarks. Benchmark::RunAll(argv[0]); - - err_msg = Dart::Cleanup(); - ASSERT(err_msg == NULL); - // Print a warning message if no tests or benchmarks were matched. if (run_matches == 0) { fprintf(stderr, "No tests matched: %s\n", run_filter); diff --git a/runtime/bin/utils.h b/runtime/bin/utils.h index 44ba6ba1d86..10ca37353c0 100644 --- a/runtime/bin/utils.h +++ b/runtime/bin/utils.h @@ -11,6 +11,7 @@ #include "include/dart_api.h" #include "platform/globals.h" + namespace dart { namespace bin { diff --git a/runtime/bin/vmservice/vmservice_io.dart b/runtime/bin/vmservice/vmservice_io.dart index d9b0c754f17..409bc5972a0 100644 --- a/runtime/bin/vmservice/vmservice_io.dart +++ b/runtime/bin/vmservice/vmservice_io.dart @@ -89,15 +89,6 @@ main() { // scheduled microtasks. Timer.run(() {}); } - // TODO(johnmccutchan, turnidge) Creating a VMService object here causes - // strange behavior from the legacy debug protocol and coverage tool. - // Enable this code, and remove the call to Isolate::KillIsolate() from - // service_isolate.cc when the strange behavior is solved. - // See: https://github.com/dart-lang/sdk/issues/23977 - // else { - // var service = new VMService(); - // service.onShutdown = _onShutdown; - // } scriptLoadPort.handler = _processLoadRequest; // Register signal handler after a small delay to avoid stalling main // isolate startup. diff --git a/runtime/include/dart_api.h b/runtime/include/dart_api.h index 41b1cb18f40..d3f54b1b48e 100755 --- a/runtime/include/dart_api.h +++ b/runtime/include/dart_api.h @@ -368,6 +368,7 @@ DART_EXPORT void _Dart_ReportErrorHandle(const char* file, } \ } \ + /** * Converts an object to a string. * @@ -860,10 +861,9 @@ typedef bool (*Dart_EntropySource)(uint8_t* buffer, intptr_t length); * \param shutdown A function to be called when an isolate is shutdown. * See Dart_IsolateShutdownCallback. * - * \return NULL if initialization is successful. Returns an error message - * otherwise. The caller is responsible for freeing the error message. + * \return True if initialization is successful. */ -DART_EXPORT char* Dart_Initialize( +DART_EXPORT bool Dart_Initialize( const uint8_t* vm_isolate_snapshot, Dart_IsolateCreateCallback create, Dart_IsolateInterruptCallback interrupt, @@ -878,10 +878,9 @@ DART_EXPORT char* Dart_Initialize( /** * Cleanup state in the VM before process termination. * - * \return NULL if cleanup is successful. Returns an error message otherwise. - * The caller is responsible for freeing the error message. + * \return True if cleanup is successful. */ -DART_EXPORT char* Dart_Cleanup(); +DART_EXPORT bool Dart_Cleanup(); /** * Sets command line flags. Should be called before Dart_Initialize. diff --git a/runtime/tests/vm/dart/spawn_infinite_loop_test.dart b/runtime/tests/vm/dart/spawn_infinite_loop_test.dart deleted file mode 100644 index cc0fb42d2a6..00000000000 --- a/runtime/tests/vm/dart/spawn_infinite_loop_test.dart +++ /dev/null @@ -1,18 +0,0 @@ -// Copyright (c) 2015, 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. - -import 'dart:isolate'; - -// This test ensures that the VM can kill the spawned isolate during VM -// shutdown even when the isolate is in an infinite loop and will not finish -// on its own. - -void loop(msg) { - while (true) {} - throw "Unreachable"; -} - -void main() { - Isolate.spawn(loop, []); -} diff --git a/runtime/tests/vm/dart/spawn_shutdown_test.dart b/runtime/tests/vm/dart/spawn_shutdown_test.dart deleted file mode 100644 index 965cf18e986..00000000000 --- a/runtime/tests/vm/dart/spawn_shutdown_test.dart +++ /dev/null @@ -1,32 +0,0 @@ -// Copyright (c) 2015, 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. - -import 'dart:async'; -import 'dart:isolate'; - -// Spawn an isolate |foo| that will continue trying to spawn isolates even after -// the timer in |main| completes. This test ensures that the VM can shutdown -// correctly even while an isolate is attempting to spawn more isolates. - -isolate1(sendPort) { - var receivePort = new ReceivePort(); - sendPort.send(receivePort.sendPort); - receivePort.listen((msg) {}); -} - -void foo(int count) { - for (int i = 0; i < count; i++) { - var receivePort = new ReceivePort(); - Isolate.spawn(isolate1, receivePort.sendPort); - receivePort.listen((sendPort) { - Isolate.spawn(isolate1,sendPort); - receivePort.close(); - }); - } -} - -void main() { - Isolate.spawn(foo, 100); - new Timer(const Duration(seconds: 10), () {}); -} diff --git a/runtime/tests/vm/vm.status b/runtime/tests/vm/vm.status index 7f82dcbfd87..d0ffb465227 100644 --- a/runtime/tests/vm/vm.status +++ b/runtime/tests/vm/vm.status @@ -88,8 +88,6 @@ dart/optimized_stacktrace_test: StaticWarning [ $runtime != vm ] dart/snapshot_version_test: SkipByDesign # Spawns processes -dart/spawn_infinite_loop_test: Skip # VM shutdown test -dart/spawn_shutdown_test: Skip # VM Shutdown test [ $runtime == vm && $mode == debug && $builder_tag == asan ] cc/Dart2JSCompileAll: Skip # Timeout. diff --git a/runtime/vm/cpuinfo_test.cc b/runtime/vm/cpuinfo_test.cc index 860c4076012..e717d28eb15 100644 --- a/runtime/vm/cpuinfo_test.cc +++ b/runtime/vm/cpuinfo_test.cc @@ -10,8 +10,10 @@ namespace dart { UNIT_TEST_CASE(GetCpuModelTest) { + CpuInfo::InitOnce(); const char* cpumodel = CpuInfo::GetCpuModel(); EXPECT_NE(strlen(cpumodel), 0UL); + CpuInfo::Cleanup(); } } // namespace dart diff --git a/runtime/vm/custom_isolate_test.cc b/runtime/vm/custom_isolate_test.cc index 2cb2e0047b8..af3050759d4 100644 --- a/runtime/vm/custom_isolate_test.cc +++ b/runtime/vm/custom_isolate_test.cc @@ -59,7 +59,6 @@ static const char* kCustomIsolateScriptChars = " var replyTo = message[1];\n" " echo('Received: $data');\n" " replyTo.send(data + 1);\n" - " mainPort.close();\n" " };\n" "}\n" "\n" @@ -180,7 +179,6 @@ void StartEvent::Process() { free(const_cast(main_)); main_ = NULL; - Dart_SetMessageNotifyCallback(NULL); Dart_ExitScope(); Dart_ExitIsolate(); } @@ -209,7 +207,6 @@ void MessageEvent::Process() { if (!Dart_HasLivePorts()) { OS::Print("<< Shutting down isolate(%p)\n", isolate()); event_queue->RemoveEventsForIsolate(isolate()); - Dart_SetMessageNotifyCallback(NULL); Dart_ShutdownIsolate(); } else { Dart_ExitScope(); @@ -353,7 +350,6 @@ UNIT_TEST_CASE(CustomIsolates) { free(const_cast(saved_echo)); delete event_queue; - event_queue = NULL; } } // namespace dart diff --git a/runtime/vm/dart.cc b/runtime/vm/dart.cc index 36a74dbb49c..8880a89ad83 100644 --- a/runtime/vm/dart.cc +++ b/runtime/vm/dart.cc @@ -14,7 +14,6 @@ #include "vm/handles.h" #include "vm/heap.h" #include "vm/isolate.h" -#include "vm/message.h" #include "vm/metrics.h" #include "vm/object.h" #include "vm/object_store.h" @@ -186,21 +185,18 @@ const char* Dart::InitOnce(const uint8_t* vm_isolate_snapshot, const char* Dart::Cleanup() { - ASSERT(Isolate::Current() == NULL); + // Shutdown the service isolate before shutting down the thread pool. + ServiceIsolate::Shutdown(); +#if 0 + // Ideally we should shutdown the VM isolate here, but the thread pool + // shutdown does not seem to ensure that all the threads have stopped + // execution before it terminates, this results in racing isolates. if (vm_isolate_ == NULL) { return "VM already terminated."; } - // Disable the creation of new isolates. - Isolate::DisableIsolateCreation(); + ASSERT(Isolate::Current() == NULL); - // Send the OOB Kill message to all remaining isolates. - Isolate::KillAllIsolates(); - - // Shutdown the service isolate before shutting down the thread pool. - ServiceIsolate::Shutdown(); - - // Shutdown the thread pool. On return, all thread pool threads have exited. delete thread_pool_; thread_pool_ = NULL; @@ -208,14 +204,19 @@ const char* Dart::Cleanup() { Thread::EnsureInit(); Thread::EnterIsolate(vm_isolate_); + // There is a planned and known asymmetry here: We exit one scope for the VM + // isolate to account for the scope that was entered in Dart_InitOnce. + Dart_ExitScope(); + ShutdownIsolate(); vm_isolate_ = NULL; TargetCPUFeatures::Cleanup(); +#endif + Profiler::Shutdown(); CodeObservers::DeleteAll(); - ASSERT(Isolate::IsolateListLength() == 0); return NULL; } @@ -224,6 +225,7 @@ Isolate* Dart::CreateIsolate(const char* name_prefix, const Dart_IsolateFlags& api_flags) { // Create a new isolate. Isolate* isolate = Isolate::Init(name_prefix, api_flags); + ASSERT(isolate != NULL); return isolate; } diff --git a/runtime/vm/dart_api_impl.cc b/runtime/vm/dart_api_impl.cc index 6880eab00e7..3ccb68b0504 100644 --- a/runtime/vm/dart_api_impl.cc +++ b/runtime/vm/dart_api_impl.cc @@ -1263,7 +1263,7 @@ DART_EXPORT const char* Dart_VersionString() { return Version::String(); } -DART_EXPORT char* Dart_Initialize( +DART_EXPORT bool Dart_Initialize( const uint8_t* vm_isolate_snapshot, Dart_IsolateCreateCallback create, Dart_IsolateInterruptCallback interrupt, @@ -1279,19 +1279,21 @@ DART_EXPORT char* Dart_Initialize( file_open, file_read, file_write, file_close, entropy_source); if (err_msg != NULL) { - return strdup(err_msg); + OS::PrintErr("Dart_Initialize: %s\n", err_msg); + return false; } - return NULL; + return true; } -DART_EXPORT char* Dart_Cleanup() { +DART_EXPORT bool Dart_Cleanup() { CHECK_NO_ISOLATE(Isolate::Current()); const char* err_msg = Dart::Cleanup(); if (err_msg != NULL) { - return strdup(err_msg); + OS::PrintErr("Dart_Cleanup: %s\n", err_msg); + return false; } - return NULL; + return true; } @@ -1362,10 +1364,6 @@ DART_EXPORT Dart_Isolate Dart_CreateIsolate(const char* script_uri, } Isolate* isolate = Dart::CreateIsolate(isolate_name, *flags); free(isolate_name); - if (isolate == NULL) { - *error = strdup("Isolate creation failed"); - return reinterpret_cast(NULL); - } { StackZone zone(isolate); HANDLESCOPE(isolate); diff --git a/runtime/vm/debugger_api_impl_test.cc b/runtime/vm/debugger_api_impl_test.cc index bd9e3e1cab4..e539374da80 100644 --- a/runtime/vm/debugger_api_impl_test.cc +++ b/runtime/vm/debugger_api_impl_test.cc @@ -1405,7 +1405,6 @@ UNIT_TEST_CASE(Debug_IsolateID) { EXPECT(Dart_GetIsolateId(isolate) == test_isolate_id); Dart_ExitScope(); Dart_ShutdownIsolate(); - Dart_SetIsolateEventHandler(NULL); EXPECT(verify_callback == 0x5); // Only created and shutdown events. } diff --git a/runtime/vm/isolate.cc b/runtime/vm/isolate.cc index 064ba77d5b5..fcbf9c48377 100644 --- a/runtime/vm/isolate.cc +++ b/runtime/vm/isolate.cc @@ -5,13 +5,11 @@ #include "vm/isolate.h" #include "include/dart_api.h" -#include "include/dart_native_api.h" #include "platform/assert.h" #include "platform/json.h" #include "vm/code_observers.h" #include "vm/compiler_stats.h" #include "vm/coverage.h" -#include "vm/dart_api_message.h" #include "vm/dart_api_state.h" #include "vm/dart_entry.h" #include "vm/debugger.h" @@ -167,6 +165,7 @@ class IsolateMessageHandler : public MessageHandler { bool IsCurrentIsolate() const; virtual Isolate* isolate() const { return isolate_; } + private: // Keep both these enums in sync with isolate_patch.dart. // The different Isolate API message types. enum { @@ -187,7 +186,6 @@ class IsolateMessageHandler : public MessageHandler { kAsEventAction = 2 }; - private: // A result of false indicates that the isolate should terminate the // processing of further events. bool HandleLibMessage(const Array& message); @@ -751,7 +749,6 @@ void Isolate::InitOnce() { create_callback_ = NULL; isolates_list_monitor_ = new Monitor(); ASSERT(isolates_list_monitor_ != NULL); - EnableIsolateCreation(); } @@ -830,14 +827,8 @@ Isolate* Isolate::Init(const char* name_prefix, result->compiler_stats_ = new CompilerStats(result); } ObjectIdRing::Init(result); - - // Add to isolate list. Shutdown and delete the isolate on failure. - if (!AddIsolateToList(result)) { - result->LowLevelShutdown(); - Thread::ExitIsolate(); - delete result; - return NULL; - } + // Add to isolate list. + AddIsolateTolist(result); return result; } @@ -1463,50 +1454,6 @@ class FinalizeWeakPersistentHandlesVisitor : public HandleVisitor { }; -void Isolate::LowLevelShutdown() { - // Ensure we have a zone and handle scope so that we can call VM functions, - // but we no longer allocate new heap objects. - StackZone stack_zone(this); - HandleScope handle_scope(this); - NoSafepointScope no_safepoint_scope; - - if (compiler_stats_ != NULL) { - compiler_stats()->Print(); - } - - // Notify exit listeners that this isolate is shutting down. - if (object_store() != NULL) { - NotifyExitListeners(); - } - - // Clean up debugger resources. - debugger()->Shutdown(); - - // Close all the ports owned by this isolate. - PortMap::ClosePorts(message_handler()); - - // Fail fast if anybody tries to post any more messsages to this isolate. - delete message_handler(); - set_message_handler(NULL); - - // Dump all accumulated timer data for the isolate. - timer_list_.ReportTimers(); - - // Finalize any weak persistent handles with a non-null referent. - FinalizeWeakPersistentHandlesVisitor visitor; - api_state()->weak_persistent_handles().VisitHandles(&visitor); - api_state()->prologue_weak_persistent_handles().VisitHandles(&visitor); - - if (FLAG_trace_isolates) { - heap()->PrintSizes(); - megamorphic_cache_table()->PrintSizes(); - Symbols::DumpStats(); - OS::Print("[-] Stopping isolate:\n" - "\tisolate: %s\n", name()); - } -} - - void Isolate::Shutdown() { ASSERT(this == Isolate::Current()); ASSERT(top_resource() == NULL); @@ -1524,10 +1471,7 @@ void Isolate::Shutdown() { HandleScope handle_scope(this); // Write out the coverage data if collection has been enabled. - if ((this != Dart::vm_isolate()) && - !ServiceIsolate::IsServiceIsolateDescendant(this)) { - CodeCoverage::Write(this); - } + CodeCoverage::Write(this); if ((timeline_event_recorder_ != NULL) && (FLAG_timeline_trace_dir != NULL)) { @@ -1550,7 +1494,48 @@ void Isolate::Shutdown() { } // Then, proceed with low-level teardown. - LowLevelShutdown(); + { + // Ensure we have a zone and handle scope so that we can call VM functions, + // but we no longer allocate new heap objects. + StackZone stack_zone(this); + HandleScope handle_scope(this); + NoSafepointScope no_safepoint_scope; + + if (compiler_stats_ != NULL) { + compiler_stats()->Print(); + } + + // Notify exit listeners that this isolate is shutting down. + if (object_store() != NULL) { + NotifyExitListeners(); + } + + // Clean up debugger resources. + debugger()->Shutdown(); + + // Close all the ports owned by this isolate. + PortMap::ClosePorts(message_handler()); + + // Fail fast if anybody tries to post any more messsages to this isolate. + delete message_handler(); + set_message_handler(NULL); + + // Dump all accumulated timer data for the isolate. + timer_list_.ReportTimers(); + + // Finalize any weak persistent handles with a non-null referent. + FinalizeWeakPersistentHandlesVisitor visitor; + api_state()->weak_persistent_handles().VisitHandles(&visitor); + api_state()->prologue_weak_persistent_handles().VisitHandles(&visitor); + + if (FLAG_trace_isolates) { + heap()->PrintSizes(); + megamorphic_cache_table()->PrintSizes(); + Symbols::DumpStats(); + OS::Print("[-] Stopping isolate:\n" + "\tisolate: %s\n", name()); + } + } #if defined(DEBUG) // No concurrent sweeper tasks should be running at this point. @@ -1583,7 +1568,7 @@ Dart_EntropySource Isolate::entropy_source_callback_ = NULL; Monitor* Isolate::isolates_list_monitor_ = NULL; Isolate* Isolate::isolates_list_head_ = NULL; -bool Isolate::creation_enabled_ = false; + void Isolate::IterateObjectPointers(ObjectPointerVisitor* visitor, bool visit_prologue_weak_handles, @@ -1932,16 +1917,12 @@ intptr_t Isolate::IsolateListLength() { } -bool Isolate::AddIsolateToList(Isolate* isolate) { +void Isolate::AddIsolateTolist(Isolate* isolate) { MonitorLocker ml(isolates_list_monitor_); - if (!creation_enabled_) { - return false; - } ASSERT(isolate != NULL); ASSERT(isolate->next_ == NULL); isolate->next_ = isolates_list_head_; isolates_list_head_ = isolate; - return true; } @@ -1963,8 +1944,7 @@ void Isolate::RemoveIsolateFromList(Isolate* isolate) { previous = current; current = current->next_; } - // If we are shutting down the VM, the isolate may not be in the list. - ASSERT(!creation_enabled_); + UNREACHABLE(); } @@ -1981,18 +1961,6 @@ void Isolate::CheckForDuplicateThreadState(InterruptableThreadState* state) { #endif -void Isolate::DisableIsolateCreation() { - MonitorLocker ml(isolates_list_monitor_); - creation_enabled_ = false; -} - - -void Isolate::EnableIsolateCreation() { - MonitorLocker ml(isolates_list_monitor_); - creation_enabled_ = true; -} - - template T* Isolate::AllocateReusableHandle() { T* handle = reinterpret_cast(reusable_handles_.AllocateScopedHandle()); @@ -2001,73 +1969,6 @@ T* Isolate::AllocateReusableHandle() { } -void Isolate::KillIsolate(Isolate* isolate) { - Dart_CObject kill_msg; - Dart_CObject* list_values[4]; - kill_msg.type = Dart_CObject_kArray; - kill_msg.value.as_array.length = 4; - kill_msg.value.as_array.values = list_values; - - Dart_CObject oob; - oob.type = Dart_CObject_kInt32; - oob.value.as_int32 = Message::kIsolateLibOOBMsg; - list_values[0] = &oob; - - Dart_CObject kill; - kill.type = Dart_CObject_kInt32; - kill.value.as_int32 = IsolateMessageHandler::kKillMsg; - list_values[1] = &kill; - - Dart_CObject cap; - cap.type = Dart_CObject_kCapability; - cap.value.as_capability.id = isolate->terminate_capability(); - list_values[2] = ∩ - - Dart_CObject imm; - imm.type = Dart_CObject_kInt32; - imm.value.as_int32 = IsolateMessageHandler::kImmediateAction; - list_values[3] = &imm; - - isolate->ScheduleInterrupts(Isolate::kMessageInterrupt); - { - uint8_t* buffer = NULL; - ApiMessageWriter writer(&buffer, allocator); - bool success = writer.WriteCMessage(&kill_msg); - ASSERT(success); - - // Post the message at the given port. - success = PortMap::PostMessage(new Message(isolate->main_port(), - buffer, - writer.BytesWritten(), - Message::kOOBPriority)); - ASSERT(success); - } -} - - -class IsolateKillerVisitor : public IsolateVisitor { - public: - IsolateKillerVisitor() {} - - virtual ~IsolateKillerVisitor() {} - - void VisitIsolate(Isolate* isolate) { - ASSERT(isolate != NULL); - if (ServiceIsolate::IsServiceIsolateDescendant(isolate) || - (isolate == Dart::vm_isolate())) { - return; - } - Isolate::KillIsolate(isolate); - } -}; - - -void Isolate::KillAllIsolates() { - IsolateKillerVisitor visitor; - VisitIsolates(&visitor); -} - - static RawInstance* DeserializeObject(Isolate* isolate, Zone* zone, uint8_t* obj_data, diff --git a/runtime/vm/isolate.h b/runtime/vm/isolate.h index a6238fd8b38..ecc59cba169 100644 --- a/runtime/vm/isolate.h +++ b/runtime/vm/isolate.h @@ -742,12 +742,6 @@ class Isolate : public BaseIsolate { mutator_thread_->set_zone(zone); } - static void KillIsolate(Isolate* isolate); - static void KillAllIsolates(); - - static void DisableIsolateCreation(); - static void EnableIsolateCreation(); - private: friend class Dart; // Init, InitOnce, Shutdown. @@ -757,7 +751,6 @@ class Isolate : public BaseIsolate { static Isolate* Init(const char* name_prefix, const Dart_IsolateFlags& api_flags, bool is_vm_isolate = false); - void LowLevelShutdown(); void Shutdown(); void BuildName(const char* name_prefix); @@ -924,14 +917,12 @@ class Isolate : public BaseIsolate { static void WakePauseEventHandler(Dart_Isolate isolate); // Manage list of existing isolates. - static bool AddIsolateToList(Isolate* isolate); + static void AddIsolateTolist(Isolate* isolate); static void RemoveIsolateFromList(Isolate* isolate); static void CheckForDuplicateThreadState(InterruptableThreadState* state); - // This monitor protects isolates_list_head_, and creation_enabled_. - static Monitor* isolates_list_monitor_; + static Monitor* isolates_list_monitor_; // Protects isolates_list_head_ static Isolate* isolates_list_head_; - static bool creation_enabled_; #define REUSABLE_FRIEND_DECLARATION(name) \ friend class Reusable##name##HandleScope; diff --git a/runtime/vm/message_handler.cc b/runtime/vm/message_handler.cc index b83c1398982..6a473dd7403 100644 --- a/runtime/vm/message_handler.cc +++ b/runtime/vm/message_handler.cc @@ -121,7 +121,7 @@ void MessageHandler::PostMessage(Message* message, bool before_events) { } message = NULL; // Do not access message. May have been deleted. - if ((pool_ != NULL) && (task_ == NULL)) { + if (pool_ != NULL && task_ == NULL) { task_ = new MessageHandlerTask(this); pool_->Run(task_); } @@ -146,7 +146,7 @@ bool MessageHandler::HandleMessages(bool allow_normal_messages, // If isolate() returns NULL StartIsolateScope does nothing. StartIsolateScope start_isolate(isolate()); - // ThreadInterrupter may have gone to sleep while waiting for + // ThreadInterrupter may have gone to sleep waiting while waiting for // an isolate to start handling messages. ThreadInterrupter::WakeUp(); diff --git a/runtime/vm/service_isolate.cc b/runtime/vm/service_isolate.cc index f866f31ed98..3779905093b 100644 --- a/runtime/vm/service_isolate.cc +++ b/runtime/vm/service_isolate.cc @@ -749,30 +749,8 @@ void ServiceIsolate::Run() { } -void ServiceIsolate::KillServiceIsolate() { - { - MonitorLocker ml(monitor_); - shutting_down_ = true; - } - Isolate::KillIsolate(isolate_); - { - MonitorLocker ml(monitor_); - while (shutting_down_) { - ml.Wait(); - } - } -} - - void ServiceIsolate::Shutdown() { if (!IsRunning()) { - if (isolate_ != NULL) { - // TODO(johnmccutchan,turnidge) When it is possible to properly create - // the VMService object and set up its shutdown handler in the service - // isolate's main() function, this case will no longer be possible and - // can be removed. - KillServiceIsolate(); - } return; } { diff --git a/runtime/vm/service_isolate.h b/runtime/vm/service_isolate.h index 86433c9a9cb..fbcd1368e0f 100644 --- a/runtime/vm/service_isolate.h +++ b/runtime/vm/service_isolate.h @@ -32,9 +32,6 @@ class ServiceIsolate : public AllStatic { static void SendServiceExitMessage(); static void Shutdown(); - private: - static void KillServiceIsolate(); - protected: static void SetServicePort(Dart_Port port); static void SetServiceIsolate(Isolate* isolate); diff --git a/runtime/vm/thread.cc b/runtime/vm/thread.cc index 9345b645a86..3b3eb3308d3 100644 --- a/runtime/vm/thread.cc +++ b/runtime/vm/thread.cc @@ -134,8 +134,11 @@ void Thread::EnterIsolate(Isolate* isolate) { ASSERT(thread->thread_state() == NULL); InterruptableThreadState* thread_state = ThreadInterrupter::GetCurrentThreadState(); - // TODO(koda): Calling Isolate::CheckForDuplicateThreadState(thread_state) - // here can lead to deadlock. Evaluate doing this check some other way. +#if defined(DEBUG) + thread->set_thread_state(NULL); // Exclude thread itself from the dupe check. + Isolate::CheckForDuplicateThreadState(thread_state); + thread->set_thread_state(thread_state); +#endif ASSERT(thread_state != NULL); // TODO(koda): Migrate profiler interface to use Thread. Profiler::BeginExecution(isolate); diff --git a/runtime/vm/thread_pool.cc b/runtime/vm/thread_pool.cc index 87f441519e4..5b3a713d2d0 100644 --- a/runtime/vm/thread_pool.cc +++ b/runtime/vm/thread_pool.cc @@ -12,6 +12,9 @@ namespace dart { DEFINE_FLAG(int, worker_timeout_millis, 5000, "Free workers when they have been idle for this amount of time."); +Monitor* ThreadPool::exit_monitor_ = NULL; +int* ThreadPool::exit_count_ = NULL; + ThreadPool::ThreadPool() : shutting_down_(false), all_workers_(NULL), @@ -19,8 +22,7 @@ ThreadPool::ThreadPool() count_started_(0), count_stopped_(0), count_running_(0), - count_idle_(0), - shutting_down_workers_(NULL) { + count_idle_(0) { } @@ -92,27 +94,14 @@ void ThreadPool::Shutdown() { } // Release ThreadPool::mutex_ before calling Worker functions. - { - MonitorLocker eml(&exit_monitor_); - - // First tell all the workers to shut down. - Worker* current = saved; - while (current != NULL) { - Worker* next = current->all_next_; - if (current->id_ != OSThread::GetCurrentThreadId()) { - AddWorkerToShutdownList(current); - } - current->Shutdown(); - current = next; - } - saved = NULL; - - // Wait until all workers have exited. - while (shutting_down_workers_ != NULL) { - // Here, we are waiting for workers to exit. When a worker exits we will - // be notified. - eml.Wait(); - } + Worker* current = saved; + while (current != NULL) { + // We may access all_next_ without holding ThreadPool::mutex_ here + // because the worker is no longer owned by the ThreadPool. + Worker* next = current->all_next_; + current->all_next_ = NULL; + current->Shutdown(); + current = next; } } @@ -168,7 +157,6 @@ bool ThreadPool::RemoveWorkerFromAllList(Worker* worker) { worker->all_next_ = NULL; worker->owned_ = false; worker->pool_ = NULL; - worker->done_ = true; return true; } @@ -218,38 +206,6 @@ bool ThreadPool::ReleaseIdleWorker(Worker* worker) { } -// Only call while holding the exit_monitor_ -void ThreadPool::AddWorkerToShutdownList(Worker* worker) { - worker->shutdown_next_ = shutting_down_workers_; - shutting_down_workers_ = worker; -} - - -// Only call while holding the exit_monitor_ -bool ThreadPool::RemoveWorkerFromShutdownList(Worker* worker) { - ASSERT(worker != NULL); - ASSERT(shutting_down_workers_ != NULL); - - // Special case head of list. - if (shutting_down_workers_ == worker) { - shutting_down_workers_ = worker->shutdown_next_; - worker->shutdown_next_ = NULL; - return true; - } - - for (Worker* current = shutting_down_workers_; - current->shutdown_next_ != NULL; - current = current->shutdown_next_) { - if (current->shutdown_next_ == worker) { - current->shutdown_next_ = worker->shutdown_next_; - worker->shutdown_next_ = NULL; - return true; - } - } - return false; -} - - ThreadPool::Task::Task() { } @@ -260,14 +216,10 @@ ThreadPool::Task::~Task() { ThreadPool::Worker::Worker(ThreadPool* pool) : pool_(pool), - done_(false), task_(NULL), - id_(OSThread::kInvalidThreadId), - started_(false), owned_(false), all_next_(NULL), - idle_next_(NULL), - shutdown_next_(NULL) { + idle_next_(NULL) { } @@ -312,7 +264,7 @@ static int64_t ComputeTimeout(int64_t idle_start) { } -bool ThreadPool::Worker::Loop() { +void ThreadPool::Worker::Loop() { MonitorLocker ml(&monitor_); int64_t idle_start; while (true) { @@ -329,9 +281,9 @@ bool ThreadPool::Worker::Loop() { ASSERT(task_ == NULL); if (IsDone()) { - return false; + return; } - ASSERT(!done_); + ASSERT(pool_ != NULL); pool_->SetIdle(this); idle_start = OS::GetCurrentTimeMillis(); while (true) { @@ -342,21 +294,21 @@ bool ThreadPool::Worker::Loop() { break; } if (IsDone()) { - return false; + return; } - if ((result == Monitor::kTimedOut) && pool_->ReleaseIdleWorker(this)) { - return true; + if (result == Monitor::kTimedOut && + pool_->ReleaseIdleWorker(this)) { + return; } } } UNREACHABLE(); - return false; } void ThreadPool::Worker::Shutdown() { MonitorLocker ml(&monitor_); - done_ = true; + pool_ = NULL; // Fail fast if someone tries to access pool_. ml.Notify(); } @@ -365,55 +317,20 @@ void ThreadPool::Worker::Shutdown() { void ThreadPool::Worker::Main(uword args) { Thread::EnsureInit(); Worker* worker = reinterpret_cast(args); - bool delete_self = false; - - { - MonitorLocker ml(&(worker->monitor_)); - if (worker->IsDone()) { - // id_ hasn't been set yet, but the ThreadPool is being shutdown. - // Delete the task, and return. - ASSERT(worker->task_); - delete worker->task_; - worker->task_ = NULL; - delete_self = true; - } else { - worker->id_ = OSThread::GetCurrentThreadId(); - worker->started_ = true; - } - } - - // We aren't able to delete the worker while holding the worker's monitor. - // Now that we have released it, and we know that ThreadPool::Shutdown - // won't touch it again, we can delete it and return. - if (delete_self) { - MonitorLocker eml(&worker->pool_->exit_monitor_); - worker->pool_->RemoveWorkerFromShutdownList(worker); - delete worker; - eml.Notify(); - return; - } - - bool released = worker->Loop(); + worker->Loop(); // It should be okay to access these unlocked here in this assert. - // worker->all_next_ is retained by the pool for shutdown monitoring. - ASSERT(!worker->owned_ && (worker->idle_next_ == NULL)); + ASSERT(!worker->owned_ && + worker->all_next_ == NULL && + worker->idle_next_ == NULL); - if (!released) { - // This worker is exiting because the thread pool is being shut down. - // Inform the thread pool that we are exiting. We remove this worker from - // shutting_down_workers_ list because there will be no need for the - // ThreadPool to take action for this worker. - MonitorLocker eml(&worker->pool_->exit_monitor_); - worker->id_ = OSThread::kInvalidThreadId; - worker->pool_->RemoveWorkerFromShutdownList(worker); - delete worker; - eml.Notify(); - } else { - // This worker is going down because it was idle for too long. This case - // is not due to a ThreadPool Shutdown. Thus, we simply delete the worker. - delete worker; + // The exit monitor is only used during testing. + if (ThreadPool::exit_monitor_) { + MonitorLocker ml(ThreadPool::exit_monitor_); + (*ThreadPool::exit_count_)++; + ml.Notify(); } + delete worker; #if defined(TARGET_OS_WINDOWS) Thread::CleanUp(); #endif diff --git a/runtime/vm/thread_pool.h b/runtime/vm/thread_pool.h index a8e212bcc25..792aef7b238 100644 --- a/runtime/vm/thread_pool.h +++ b/runtime/vm/thread_pool.h @@ -5,7 +5,6 @@ #ifndef VM_THREAD_POOL_H_ #define VM_THREAD_POOL_H_ -#include "vm/allocation.h" #include "vm/globals.h" #include "vm/os_thread.h" @@ -30,7 +29,7 @@ class ThreadPool { ThreadPool(); - // Shuts down this thread pool. Causes workers to terminate + // Shuts down this thread pool. Causes workers to terminate // themselves when they are active again. ~ThreadPool(); @@ -44,6 +43,8 @@ class ThreadPool { uint64_t workers_stopped() const { return count_stopped_; } private: + friend class ThreadPoolTestPeer; + class Worker { public: explicit Worker(ThreadPool* pool); @@ -55,31 +56,24 @@ class ThreadPool { // after a task has been set by the initial call to SetTask(). void StartThread(); - // Main loop for a worker. Returns true if worker is removed from thread - // lists, false otherwise. - bool Loop(); + // Main loop for a worker. + void Loop(); // Causes worker to terminate eventually. void Shutdown(); - // Get the Worker's thread id. - ThreadId id() { return id_; } - private: friend class ThreadPool; // The main entry point for new worker threads. static void Main(uword args); - bool IsDone() const { return done_; } + bool IsDone() const { return pool_ == NULL; } // Fields owned by Worker. Monitor monitor_; ThreadPool* pool_; - bool done_; Task* task_; - ThreadId id_; - bool started_; // Fields owned by ThreadPool. Workers should not look at these // directly. It's like looking at the sun. @@ -87,8 +81,6 @@ class ThreadPool { Worker* all_next_; // Protected by ThreadPool::mutex_ Worker* idle_next_; // Protected by ThreadPool::mutex_ - Worker* shutdown_next_; // Protected by ThreadPool::exit_monitor - DISALLOW_COPY_AND_ASSIGN(Worker); }; @@ -100,9 +92,6 @@ class ThreadPool { bool RemoveWorkerFromIdleList(Worker* worker); bool RemoveWorkerFromAllList(Worker* worker); - void AddWorkerToShutdownList(Worker* worker); - bool RemoveWorkerFromShutdownList(Worker* worker); - // Worker operations. void SetIdle(Worker* worker); bool ReleaseIdleWorker(Worker* worker); @@ -116,8 +105,8 @@ class ThreadPool { uint64_t count_running_; uint64_t count_idle_; - Monitor exit_monitor_; - Worker* shutting_down_workers_; + static Monitor* exit_monitor_; // Used only in testing. + static int* exit_count_; // Used only in testing. DISALLOW_COPY_AND_ASSIGN(ThreadPool); }; diff --git a/runtime/vm/thread_pool_test.cc b/runtime/vm/thread_pool_test.cc index af9f618e7ed..35a61273736 100644 --- a/runtime/vm/thread_pool_test.cc +++ b/runtime/vm/thread_pool_test.cc @@ -12,6 +12,18 @@ namespace dart { DECLARE_FLAG(int, worker_timeout_millis); +class ThreadPoolTestPeer { + public: + // When the pool has an exit monitor, workers notify a monitor just + // before they exit. This is only used in tests to make sure that + // Shutdown works. + static void SetExitMonitor(Monitor* exit_monitor, int* exit_count) { + ThreadPool::exit_monitor_ = exit_monitor; + ThreadPool::exit_count_ = exit_count; + } +}; + + UNIT_TEST_CASE(ThreadPool_Create) { ThreadPool thread_pool; } @@ -76,74 +88,40 @@ UNIT_TEST_CASE(ThreadPool_RunMany) { class SleepTask : public ThreadPool::Task { public: - explicit SleepTask( - Monitor* sync, int* started_count, int* slept_count, int millis) - : sync_(sync), - started_count_(started_count), - slept_count_(slept_count), - millis_(millis) { + explicit SleepTask(int millis) + : millis_(millis) { } virtual void Run() { - { - MonitorLocker ml(sync_); - *started_count_ = *started_count_ + 1; - ml.Notify(); - } - // Sleep so we can be sure the ThreadPool destructor blocks until we're - // done. OS::Sleep(millis_); - { - MonitorLocker ml(sync_); - *slept_count_ = *slept_count_ + 1; - // No notification here. The main thread is blocked in ThreadPool - // shutdown waiting for this thread to finish. - } } private: - Monitor* sync_; - int* started_count_; - int* slept_count_; int millis_; }; UNIT_TEST_CASE(ThreadPool_WorkerShutdown) { - const int kTaskCount = 10; - Monitor sync; - int slept_count = 0; - int started_count = 0; + Monitor exit_sync; + int exit_count = 0; + MonitorLocker ml(&exit_sync); // Set up the ThreadPool so that workers notify before they exit. ThreadPool* thread_pool = new ThreadPool(); + ThreadPoolTestPeer::SetExitMonitor(&exit_sync, &exit_count); // Run a single task. - for (int i = 0; i < kTaskCount; i++) { - thread_pool->Run(new SleepTask(&sync, &started_count, &slept_count, 2)); - } + thread_pool->Run(new SleepTask(2)); - { - // Wait for everybody to start. - MonitorLocker ml(&sync); - while (started_count < kTaskCount) { - ml.Wait(); - } - } - - // Kill the thread pool while the workers are sleeping. + // Kill the thread pool. delete thread_pool; thread_pool = NULL; - int final_count = 0; - { - MonitorLocker ml(&sync); - final_count = slept_count; + // Wait for the workers to terminate. + while (exit_count == 0) { + ml.Wait(); } - - // We should have waited for all the workers to finish, so they all should - // have had a chance to increment slept_count. - EXPECT_EQ(kTaskCount, final_count); + EXPECT_EQ(1, exit_count); } @@ -194,11 +172,12 @@ class SpawnTask : public ThreadPool::Task { // Spawn 0-2 children. if (todo_ > 0) { - pool_->Run(new SpawnTask( - pool_, sync_, todo_ - child_todo, total_, done_)); + pool_->Run( + new SpawnTask(pool_, sync_, todo_ - child_todo, total_, done_)); } if (todo_ > 1) { - pool_->Run(new SpawnTask(pool_, sync_, child_todo, total_, done_)); + pool_->Run( + new SpawnTask(pool_, sync_, child_todo, total_, done_)); } { @@ -235,4 +214,5 @@ UNIT_TEST_CASE(ThreadPool_RecursiveSpawn) { EXPECT_EQ(kTotalTasks, done); } + } // namespace dart diff --git a/runtime/vm/verified_memory_test.cc b/runtime/vm/verified_memory_test.cc index 90c21ec7fa7..6d972167f94 100644 --- a/runtime/vm/verified_memory_test.cc +++ b/runtime/vm/verified_memory_test.cc @@ -15,20 +15,12 @@ void Init() { } -void Shutdown() { -#if defined(DEBUG) - FLAG_verified_mem = false; -#endif -} - - UNIT_TEST_CASE(VerifiedMemoryReserve) { Init(); const intptr_t kReservationSize = 64 * KB; VirtualMemory* vm = VerifiedMemory::Reserve(kReservationSize); EXPECT_EQ(kReservationSize, vm->size()); delete vm; - Shutdown(); } @@ -39,7 +31,6 @@ UNIT_TEST_CASE(VerifiedMemoryCommit) { EXPECT_EQ(kReservationSize, vm->size()); vm->Commit(false); delete vm; - Shutdown(); } @@ -62,7 +53,6 @@ UNIT_TEST_CASE(VerifiedMemoryBasic) { *unverified = 123; VerifiedMemory::Verify(reinterpret_cast(addr), 3 * sizeof(double)); delete vm; - Shutdown(); } @@ -82,7 +72,6 @@ UNIT_TEST_CASE(VerifiedMemoryAccept) { VerifiedMemory::Accept(reinterpret_cast(addr), 2 * sizeof(double)); VerifiedMemory::Verify(reinterpret_cast(addr), 3 * sizeof(double)); delete vm; - Shutdown(); } @@ -97,7 +86,6 @@ UNIT_TEST_CASE(VerifyImplicit_Crash) { double* addr = reinterpret_cast(vm->address()); addr[0] = 0.5; // Forget to use Write. VerifiedMemory::Write(&addr[0], 1.5); - Shutdown(); } @@ -113,7 +101,6 @@ UNIT_TEST_CASE(VerifyExplicit_Crash) { addr[1] = 3.5; // Forget to use Write. VerifiedMemory::Write(&addr[2], 2.5); VerifiedMemory::Verify(reinterpret_cast(addr), 3 * sizeof(double)); - Shutdown(); } } // namespace dart diff --git a/tests/isolate/nested_spawn2_test.dart b/tests/isolate/nested_spawn2_test.dart index faf105dce1f..fbeaeef7c4a 100644 --- a/tests/isolate/nested_spawn2_test.dart +++ b/tests/isolate/nested_spawn2_test.dart @@ -57,7 +57,7 @@ void main([args, port]) { test("spawned isolate can spawn other isolates", () { ReceivePort init = new ReceivePort(); Isolate.spawn(isolateA, init.sendPort); - return init.first.then(expectAsync((port) { + init.first.then(expectAsync((port) { _call(port, "launch nested!", expectAsync((msg, replyTo) { expect(msg[0], "0"); _call(replyTo, msg1, expectAsync((msg, replyTo) { diff --git a/tests/isolate/nested_spawn_test.dart b/tests/isolate/nested_spawn_test.dart index 083a94a9aa4..0dc35a07bc8 100644 --- a/tests/isolate/nested_spawn_test.dart +++ b/tests/isolate/nested_spawn_test.dart @@ -24,7 +24,7 @@ void main([args, port]) { test("spawned isolates can spawn nested isolates", () { ReceivePort port = new ReceivePort(); Isolate.spawn(isolateA, [port.sendPort, "main"]); - return port.first.then((message) { + port.first.then((message) { expect("main", message[1]); expect("isolateA", message[2]); expect("isolateB", message[3]);