From d8f8723577583d0d3d0239013c6e503c7d4e3abb Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 19 Jun 2020 23:19:25 +0200 Subject: [PATCH] src: reduce scope of code cache mutex As indicated in the added comment, this can lead to a deadlock otherwise. In the concrete instance in which I encountered this, the relevant nested call is the one to `require('internal/tty')` inside of the `afterInspector()` function for uncaught exception handling. PR-URL: https://github.com/nodejs/node/pull/33980 Reviewed-By: James M Snell Reviewed-By: Ben Noordhuis Reviewed-By: Gus Caplan Reviewed-By: David Carlier Reviewed-By: Zeyu Yang Reviewed-By: Joyee Cheung --- src/node_native_module.cc | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/node_native_module.cc b/src/node_native_module.cc index 8791c99a0657fd..17abd057eaa06d 100644 --- a/src/node_native_module.cc +++ b/src/node_native_module.cc @@ -263,10 +263,13 @@ MaybeLocal NativeModuleLoader::LookupAndCompile( Local column_offset = Integer::New(isolate, 0); ScriptOrigin origin(filename, line_offset, column_offset, True(isolate)); - Mutex::ScopedLock lock(code_cache_mutex_); - ScriptCompiler::CachedData* cached_data = nullptr; { + // Note: The lock here should not extend into the + // `CompileFunctionInContext()` call below, because this function may + // recurse if there is a syntax error during bootstrap (because the fatal + // exception handler is invoked, which may load built-in modules). + Mutex::ScopedLock lock(code_cache_mutex_); auto cache_it = code_cache_.find(id); if (cache_it != code_cache_.end()) { // Transfer ownership to ScriptCompiler::Source later. @@ -313,8 +316,13 @@ MaybeLocal NativeModuleLoader::LookupAndCompile( ScriptCompiler::CreateCodeCacheForFunction(fun)); CHECK_NOT_NULL(new_cached_data); - // The old entry should've been erased by now so we can just emplace - code_cache_.emplace(id, std::move(new_cached_data)); + { + Mutex::ScopedLock lock(code_cache_mutex_); + // The old entry should've been erased by now so we can just emplace. + // If another thread did the same thing in the meantime, that should not + // be an issue. + code_cache_.emplace(id, std::move(new_cached_data)); + } return scope.Escape(fun); }