Skip to content

Commit

Permalink
src: reduce scope of code cache mutex
Browse files Browse the repository at this point in the history
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: #33980
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
  • Loading branch information
addaleax authored and danbev committed Jun 24, 2020
1 parent 8ada275 commit d8f8723
Showing 1 changed file with 12 additions and 4 deletions.
16 changes: 12 additions & 4 deletions src/node_native_module.cc
Expand Up @@ -263,10 +263,13 @@ MaybeLocal<Function> NativeModuleLoader::LookupAndCompile(
Local<Integer> 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.
Expand Down Expand Up @@ -313,8 +316,13 @@ MaybeLocal<Function> 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);
}
Expand Down

0 comments on commit d8f8723

Please sign in to comment.