Skip to content

Commit b81a643

Browse files
dmelikyanJulien Gilli
authored and
Julien Gilli
committed
V8: avoid deadlock when profiling is active
A deadlock happens when sampler initiated by SIGPROF tries to lock the thread and the thread is already locked by the same thread. As a result, other thread involved in sampling process hangs. The patch adds a check for thread lock before continuing sampler operation. The fix has been tested on a sample app under load with and without profiling turned on. Fixes issue #14576 and specifically the duplicate issue #25295 Reviewed-By: Julien Gilli <julien.gilli@joyent.com> PR-URL: nodejs/node-v0.x-archive#25309
1 parent 1034982 commit b81a643

File tree

3 files changed

+70
-0
lines changed

3 files changed

+70
-0
lines changed

deps/v8/src/isolate.h

+5
Original file line numberDiff line numberDiff line change
@@ -384,6 +384,11 @@ class Isolate {
384384
public:
385385
~Isolate();
386386

387+
// ISSUE-14576: allow to access process_wide_mutex_ from sampler.cc
388+
static base::Mutex* GetProcessWideMutexPointer() {
389+
return process_wide_mutex_.Pointer();
390+
}
391+
387392
// A thread has a PerIsolateThreadData instance for each isolate that it has
388393
// entered. That instance is allocated when the isolate is initially entered
389394
// and reused on subsequent entries.

deps/v8/src/sampler.cc

+8
Original file line numberDiff line numberDiff line change
@@ -342,6 +342,14 @@ void SignalHandler::HandleProfilerSignal(int signal, siginfo_t* info,
342342
// We require a fully initialized and entered isolate.
343343
return;
344344
}
345+
346+
// ISSUE-14576: To avoid deadlock, return if there is a thread lock
347+
if (Isolate::GetProcessWideMutexPointer()->TryLock()) {
348+
Isolate::GetProcessWideMutexPointer()->Unlock();
349+
} else {
350+
return;
351+
}
352+
345353
if (v8::Locker::IsActive() &&
346354
!isolate->thread_manager()->IsLockedByCurrentThread()) {
347355
return;

test/simple/test-regress-GH-14576.js

+57
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
// Copyright Joyent, Inc. and other Node contributors.
2+
//
3+
// Permission is hereby granted, free of charge, to any person obtaining a
4+
// copy of this software and associated documentation files (the
5+
// "Software"), to deal in the Software without restriction, including
6+
// without limitation the rights to use, copy, modify, merge, publish,
7+
// distribute, sublicense, and/or sell copies of the Software, and to permit
8+
// persons to whom the Software is furnished to do so, subject to the
9+
// following conditions:
10+
//
11+
// The above copyright notice and this permission notice shall be included
12+
// in all copies or substantial portions of the Software.
13+
//
14+
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
15+
// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
16+
// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
17+
// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
18+
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
19+
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
20+
// USE OR OTHER DEALINGS IN THE SOFTWARE.
21+
22+
/*
23+
* Tests for sampler deadlock regression.
24+
* Issue https://github.com/joyent/node/issues/14576
25+
*/
26+
27+
var assert = require('assert');
28+
var cp = require('child_process');
29+
30+
var child = undefined;
31+
32+
if (process.platform !== 'win32') {
33+
process.on('SIGTERM', function() {
34+
if(child) {
35+
process.kill(child.pid, 'SIGKILL');
36+
}
37+
38+
process.exit();
39+
});
40+
}
41+
42+
var testScript = "var i = 0; function r() { if(++i > 25) return; " +
43+
"setTimeout(r, 1); }; r();"
44+
var nodeCmd = process.execPath +
45+
' --prof --nologfile_per_isolate' +
46+
' -e "' + testScript + '"';
47+
var runs = 0;
48+
49+
function runTestScript() {
50+
child = cp.exec(nodeCmd, function(err, stdout, stderr) {
51+
if (++runs > 50) return;
52+
53+
runTestScript();
54+
});
55+
}
56+
57+
runTestScript();

0 commit comments

Comments
 (0)