chrisa / node-dtrace-provider

Native DTrace probes for node.js apps
Other
319 stars 70 forks source link

Failed assertion when firing unregistered probe name #120

Closed watsoncj closed 6 years ago

watsoncj commented 6 years ago
$ node -v
v8.11.2
$ cat dtrace.js
var dtrace = require('dtrace-provider');
var dtraceProvider = dtrace.createDTraceProvider('taniumjs');

dtraceProvider.addProbe('my-probe', 'char *');
dtraceProvider.enable();

dtraceProvider.fire('other-probe', function () {});
$ node dtrace.js
Assertion failed: (object->InternalFieldCount() > 0), function Unwrap, file ../../../nan/nan_object_wrap.h, line 33.
Abort trap: 6

This causes the VM to exit with code 134 and can be a quite difficult to track down. An appropriate error should be thrown instead.

relevant line in dtrace-provider relevant line in nodejs/nan

Thanks for the awesome project!

melloc commented 6 years ago

I'd love to make the behaviour to throw an error with a helpful message here, but it seems that today, depending on the node version being used, the process either crashes on the Unwrap or we return here. For example, in v0.10 the following test passes against current master:

var test = require('tap').test;
var fmt = require('util').format;
var d = require('../dtrace-provider');

test('firing non-existent probes', function (t) {
    var provider = d.createDTraceProvider("nodeapp");
    provider['typenull'] = null;
    provider['typenan'] = provider;

    function cb(p) {
        return [1, 2, 3, 4];
    }

    function tryName(name) {
        console.log(name);
        provider.fire(name, cb);
        t.pass(fmt('fire("%s", cb) should not raise SIGABRT', name));
    }

    tryName('kaboom');
    tryName('typenull');
    tryName('typenan');

    t.end();
});

I'm going to update fire() to behave consistently across all node versions (by just doing nothing) and make the backwards-incompatible change to throw in a future release.

I've tested these changes with:

I've published dtrace-provider@0.8.7 with the fix.