Skip to content

Remove Isolate::IsDead check from the time profiler signal handler #253

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
szegedi merged 1 commit into from
Jan 15, 2026
+1 −1
Merged

Remove Isolate::IsDead check from the time profiler signal handler #253

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bindings/profilers/wall.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,7 @@ void SignalHandler::HandleProfilerSignal(int sig,
return;
}
auto isolate = Isolate::GetCurrent();
if (!isolate || isolate->IsDead()) {
if (!isolate) {
Copy link

@r1viollet r1viollet Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@szegedi szegedi Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@r1viollet r1viollet Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK to try this, we might learn something from the newer crash location.

return;
}
WallProfiler* prof = g_profilers.GetProfiler(isolate);
Expand Down