Remove Isolate::IsDead check from the time profiler signal handler#253
Remove Isolate::IsDead check from the time profiler signal handler#253
Isolate::IsDead check from the time profiler signal handler#253Conversation
Overall package sizeSelf size: 1.77 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | source-map | 0.7.6 | 185.63 kB | 185.63 kB | | pprof-format | 2.2.1 | 163.06 kB | 163.06 kB | | p-limit | 3.1.0 | 7.75 kB | 13.78 kB | | delay | 5.0.0 | 11.17 kB | 11.17 kB | | node-gyp-build | 3.9.0 | 8.81 kB | 8.81 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
| } | ||
| auto isolate = Isolate::GetCurrent(); | ||
| if (!isolate || isolate->IsDead()) { | ||
| if (!isolate) { |
There was a problem hiding this comment.
This looks like a race cleaning up / moving the isolate.
I'm not sure that removing the IsDead will not just move this to the next line.
There was a problem hiding this comment.
Isolate doesn't move in memory, it's part of the V8 core and not an object on the JavaScript heap.
Next line is a lookup in our isolate->profiler map. If we already removed the isolate binding from the map, then it'll not find a profiler, and just delegate to the V8 handler. If it crashes in there, at least we know the issue would most likely persist even if someone used only the Node.js built-in profiler. So I'm not saying we'll eliminate the crash here, but we'll move it out of Datadog code.
There was a problem hiding this comment.
OK to try this, we might learn something from the newer crash location.
What does this PR do?:
Removes the
Isolate::IsDead()check from the time profiler signal handler.Motivation:
We're seeing SIGSEGV when
IsDeadis invoked from our signal handler. It's weird on the surface of it, because if we get back a non-nullv8::Isolate*fromIsolate::GetCurrent()call, anIsDead()check should complete successfully on it. However, in this particular case, the PROF signal is triggered whilemunmapis on the stack, it might somehow contribute to it.Additional Notes:
This
IsDead()check was added speculatively with #197 and there was never evidence that it actually helps matters. It's quite possible that under the same conditions, we'll have a crash further down the line when we invoke V8's original handler, but then we can be certain that the issue is somewhere in the lifecycle management of the isolate and not inside our handler.Jira: PROF-13457