-
Notifications
You must be signed in to change notification settings - Fork 74
Allow specifying a callback #11
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.
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
base: master
Are you sure you want to change the base?
Conversation
Interesting. Originally, I had kept the code minimal ... since there was a segfault, I wanted to make as few assumptions about the integrity of runtime state as possible. Do you have an argument for why a JS callback is safe to run here? Sorry it took me so long to notice this PR. |
pid = getpid(); | ||
snprintf(sbuff, sizeof(sbuff), "stacktrace-%d-%d.log", (int)now, pid ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not to remove these codepaths entirely. Perhaps there could be a config value to enable the default writing behavior, which would execute first, followed by the callback.
My concern here is that after a SIGSEGV and more importantly, the interrupt of V8, the execution state is not trustable. For example, the SIGSEGV might have occurred in the middle of some complicated operation that was itself in the middle of a deep stack of JS methods. V8 could have locks that it is holding or some other transient state. Calling back into the V8 interpreter at that point could produce an unpredictable result and result in the original error not being logged properly. On the other hand, presumably, the SIGSEGV is happening inside of a module and that module would have the ability to call a callback, so maybe the danger isn't as much as I fear. Still, at a minimum, I'd want to have a fallback plan with a stupid simple codepath like the one here - no allocations, no external state, no calls into V8, just a dirt-simple dump of the stacktrace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do get the value of a JS callback - being able to report the segfault to a central logging service would be super cool. I'm just paranoid and want a second copy written on the local filesystem incase everything goes sideways before the log line gets written out. The issue I was debugging when I wrote this code originally only reproduced after a few days of production usage, so I needed to be certain I'd capture the stack the next time it happened, even if it required SSHing into the box that died.
I can tell you from practice that I successfully store stacktrace into MongoDB when it occurs. So it seems it is safe enough. :-) |
The point is that the whole signal handling uses whole separate stack anyway. So wherever fault occurred, this is another stack, another path of execution. When you get to signal handler, you have more or less a clean slate. And if you really want to capture everything, even your approach is not the best one. It would be better to run your command with something like this. |
So I think that module is nice for things where you can still be in JavaScript. But if you really want paranoid version, you should do it from outside. |
Updated this one as well. |
I really like this feature but this pull is not compatible with the updates since. |
Made it so that you can specify a callback and you get a stacktrace in it. You can then write to the file yourself if you want or do anything else (write to the database).