Debugging query callback with node-inspector! #62

Closed
clintwood opened this Issue Aug 22, 2012 · 19 comments

5 participants

@clintwood

When using node-inspector for debugging, setting a breakpoint in a callback function body of any callback function that goes across the OdbcConnectionBridge causes node to terminate without the breakpoint being hit.

You can reproduce this by setting a breakpoint anywhere in the body of the callback function as indicated below.

// Query with explicit connection
var sql = require('node-sqlserver');
var conn_str = "Driver={SQL Server Native Client 11.0};Server={(LocalDb)\\v11.0};Database={NodeDB};Trusted_Connection={Yes};";

sql.open(conn_str, function (err, conn) {
    // Setting a debug breakpoint anywhere between here ...
    if (err) {
        console.log("Error opening the connection!");
        return;
    }
    conn.queryRaw("SELECT * FROM NodeTable", function (err, results) {
        if (err) {
            console.log("Error running query!");
            return;
        }
        for (var i = 0; i < results.rows.length; i++) {
            console.log("0:" + results.rows[i][0]);
        }
    });
    // ... and here, will cause node to exit prematurely without breaking in the debugger.
});

I'm not convinced that this is a node-sqlserver issue per se but I see a SO question on the same thing and thought I'd post here first.

Any pointers on how to get this working would be great!

@jguerin

Hi @clintwood,

Do you have any experience with C++ debugging? Would you be able to attach a debugger to the process so that we can get more information about the crash?

Cheers,

Jonathan

@clintwood

Hi @jguerin,

Thanks for the response and sure, I will take a look at that!

Btw, to verify that this was not a general node/node-inspector issue I tried what I thought would run through the same code path (i.e. node process -> javascript -> native module -> javascript (callback)) ... with the following code:

var express = require('express')
    , fs = require('fs')
    , app = express();

app.get('/', function(req, res) {
    console.log('Doing nothing...');

    fs.readFile('./robots.txt', null, function(err, data) {
        res.type('text/plain');
        res.send(data);
    });
});

I could set a breakpoint anywhere and the debugging experience was as expected.

Clinton

@clintwood

Hi @jguerin,

I've spent some time looking at this and it is unclear why this is happening due to unfamiliarity with the v8 engine code (which is where the issue originates - so I believe), however, I have some data that may help whoever further track this down:

I am using the latest version (0.1.0) of node-sqlserver off git and a debug build of node 0.8.6.
I'm using debug builds of node-sqlserver and node itself.

So, after a call from line 55 in Operation.h...

    // Operation.h
    static void OnForeground(uv_work_t* work)
    {
        Operation* operation = (Operation*)work->data;
55      operation->CompleteForeground();
        delete operation;
    }

an exception occurs occurs at line 64 in OdbcOperation.cpp

    // OdbcOperation.cpp
    void OdbcOperation::CompleteForeground()
    {
          ....
64      callback->Call(Undefined().As<Object>(), argc, args);
          ....
    }

The exception is from inside the v8 engine. On a debug build of node.exe an assert at line 678 in frames.h occurs n the engine code in frames.h:

      // frames.h (v8 src)
      StackFrame* frame() const {
678     ASSERT(!done());  // <-- asserts here
        return frame_;
  }

Specifics: This issue only occurs when a breakpoint is set in the callback of 'sql.open' or 'sql.queryRaw' from a node-inspector debug session.

I have a node dump from the debug build of node.exe which I will email you...

My gut feel is that this is a debugger issue...

I'm not sure how else I can help?

Clinton

@jguerin

@clintwood thanks for looking into this! When you say 'latest' version, are you using the master or develop branch?

Cheers,

Jonathan

@clintwood

Hi @jguerin,

I'm using the master branch... I will try the develop branch tomorrow and see it this exhibits the same behaviour!

Clint

@jguerin

@clintwood great! We've made a lot of fixes in the develop branch, so I hope that your issue has already been addressed.

@clintwood

Hi @jguerin,

I have now tried the latest code on the develop branch and it exhibits the same issue (with a different node dump of course).

It would be great to have a debugger while using node-sqlserver, however, for now I will have to resort to console.log(...) statements!

If I get time I will try simulate your use of background threads (i.e. uv_queue_work(...)) to see if this is a general issue with node-inspector / node or if it is specific to node-sqlserver.

Clint

@icosahedron

That is unfortunate. I have a hunch about what it might be, but we will look into it as well. Thanks for your efforts.

@jkint jkint was assigned Aug 28, 2012
@clintwood

Hi @icosahedron, @jguerin,

I built a simple native c++ node module that used uv_queue_work(...) based on the approach used in node-sqlserver module and it initially exhibited the same issue.

However, changing the following code in method void OdbcOperation::CompleteForeground() seemed to fix the problem!

Code changed from:

            callback->Call(Undefined().As<Object>(), argc, args); // Current code

to:

            callback->Call(Context::GetCurrent()->Global(), argc, args); // Changed code

Warning: I have not fully tested this for memory leaks etc. but at least I can set breakpoints (via node-inspector) in any of the callbacks from the module without the crash that I originally experienced.

I hope this helps with this issue.

Clinton

@jguerin

@clintwood,

Nice find! Given we don't want to steal your thunder, we would love to see you commit this change as a contribution to this project! ;)

If it's not possible right now, we will definitely look into this for our next release and take it on as a bug.

Cheers,

Jonathan

@clintwood

@jguerin,

I believe I only have readonly access to the develop branch... you're most welcome to claim it as yours!

Clinton

@jkint

@clintwood,

I believe he means that we would welcome a pull request if you wanted to fork the repo.

@jguerin

@clintwood Github actually operates by forking a repository into your own profile, then issuing a pull request so that the maintainers can give you feedback, or ask questions about the implementation. :)

@clintwood

Aah! Sorry I'm a noob on the git front will go ahead and fork away!

@clintwood clintwood pushed a commit to clintwood/node-sqlserver that referenced this issue Aug 30, 2012
clintwood Breakpoint in callback teminates node (#62)
Fix for issue #62 where placing a breakpoint in the callback of any
node-sqlserver functions (such as sql.query(..)) caused node to
terminate.
ea8ec5e
@clintwood clintwood pushed a commit to clintwood/node-sqlserver that referenced this issue Aug 30, 2012
clintwood Breakpoint in callback teminates node (#62)
Fix for issue #62 where placing a breakpoint in the callback of any
node-sqlserver functions (such as sql.query(..)) caused node to
terminate.
386cb3b
@DMGambone

Thanks for the fix, Clintwood. This fixed a problem I was having in WebStorm trying to debug the calls!

@jguerin any thoughts on when this is going to be incorporated into the Master?

@jguerin

@DMGambone Clintwood had a pull request open, but it became inactive, so I had to close it. Feel free to have a read of the contribution guidelines and issue your own pull request. :)

@DMGambone

Unfortunately, I'm not very good with GitHub or how to contribute. Something on my TODO list. Wish I could help out.

@DMGambone DMGambone added a commit that referenced this issue Oct 16, 2012
@DMGambone DMGambone Fix for issue #62 - Debugging query callback with node-inspector!
Per the comments from clintwood, the original line:
callback->Call(Undefined().As<Object>(), argc, args);
causes node-inspector to crash if breakpoints are added in any query
callback function.  This change corrects that.
a6d5f4c
@clintwood

I'm a noob at GitHub too, how does a pull request become inactive!?

@DMGambone thanks for 're-activating' the pull request!

@jguerin

@clintwood we just released v0.2.0 (blog post on that soon), and I had to triage out any issues that were still open, but not seeing any activity. We welcome any pull requests, but we can't keep them open forever! :)

@jguerin jguerin closed this Oct 22, 2012
@jkint jkint was unassigned by clintwood Apr 1, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment