Node: Debugger protocol hangs in some special situations

[3y3]

This test fails because we don't receive nothing except first startup message

var app = require('child_process')
  .exec('node --debug -e "setInterval(console.log, 10, {a: 1})"');

app.stderr.on('data', () =>
  setTimeout(() =>
    require('net').connect('5858', function() {
      var buffer = '';
      var data = JSON.stringify({
        "seq":2,
        "type":"request",
        "command":"evaluate",
        "arguments":{
          "expression":"1",
          "global":true,
          "maxStringLength":-1
        }});

      this.on('data', (data) => buffer += data);

      this.write(`Content-Length: ${Buffer.byteLength(data, 'utf8')}\r\n\r\n${data}`);

      setTimeout(() => {
        var cl = 0;
        var rx = /Content-Length/g;

        // We wait two messages - startup and evaluation response
        // We wait two Content-Length heades - one for each message
        while (rx.exec(buffer)) cl++;
        console.assert(cl == 2, 'There is no second message');
        console.log('All works fine');
        process.exit(0);
      }, 300)
    }), 300 /* small delay to initialize debugger in app */));

But if I change console.log arguments in line 2 - all works fine:

var app = require('child_process')
  .exec('node --debug -e "setInterval(console.log, 10, 1)"');

app.stderr.on('data', () =>
  setTimeout(() =>
    require('net').connect('5858', function() {
      var buffer = '';
      var data = JSON.stringify({
        "seq":2,
        "type":"request",
        "command":"evaluate",
        "arguments":{
          "expression":"1",
          "global":true,
          "maxStringLength":-1
        }});

      this.on('data', (data) => buffer += data);

      this.write(`Content-Length: ${Buffer.byteLength(data, 'utf8')}\r\n\r\n${data}`);

      setTimeout(() => {
        var cl = 0;
        var rx = /Content-Length/g;

        // We wait two messages - startup and evaluation response
        // We wait two Content-Length heades - one for each message
        while (rx.exec(buffer)) cl++;
        console.assert(cl == 2, 'There is no second message');
        console.log('All works fine');
        process.exit(0);
      }, 300)
    }), 300 /* small delay to initialize debugger in app */));

[3y3]

This is issue for node 5.4

[dan-tull]

I found this issue because I was seeing frequent crashes of node when trying to debug, which I took to be this issue: https://github.com/nodejs/node/issues/4322

Originally, I was using 5.3.0 (latest from homebrew) and can confirm it also happens in 4.2.6 (the current node4-lts version, also via homebrew).

I cherry-picked only the small applied fix for the segfaults: https://github.com/nodejs/node/pull/4328 onto a checkout of the 4.2.6 branch and now I get the symptoms from this issue instead (my debugger hangs frequently instead of crashing frequently).

I haven't waded into the debugging protocol code yet, but my current suspicion is that the early return added to dodge the segfault is causing the hangs. Previously, the Agent::MessageHandler always took a message and enqueued a new message, but now it sometimes does nothing, which definitely sounds like the kind of change that could cause a hang (a message to which no response is sent).

It may also be at the root of this issue: https://github.com/nodejs/node/issues/4651, but I haven't tried a before/after test with the segfault fix above for that, yet.

[MylesBorins]

perhaps the fix in #4819 might help with this

[MylesBorins]

@3y3 I am so happy!!! It looks like my fix at #4819 fixes the problem you are having. I was struggling to figure out how to write a test for the regression, but your example works! Going to add the test now so we can get the problem fixed. Would you like to be added as the author of this test?

[dan-tull]

@TheAlphaNerd Just as added confirmation, if I also apply your commit in #4819 to my v4.2.6 branch and rebuild, I can successfully debug the code that originally was crashing and later hanging.

I don't know how much longer the 4.2.x branch is slated to live, but those two patches seem like candidates for inclusion in the next dot.

Thanks! It's great to have debugging more stable again.

[MylesBorins]

@dan-tull I've added https://github.com/nodejs/node/commit/25776f3ea14fb0aadae38ce91c2fbcfa95746255 to the v4.x-staging branch

Once the other PR has a working test and is landed on master we'll get the backport going

[MylesBorins]

@3y3 I ended up having to do quite a bit of modifications to get the test in a place where I felt it was ready to be submitted to core. For now I'm going to squash it into my existed PR to minimize the number of commits (easier to backport). Let me know if you have a problem with that.

[dan-tull]

@TheAlphaNerd One update: it was nagging me that I hadn't ever tested your "do not incept debug context" change in isolation, so I made a new v4.2.6 branch, built and reproduced the crash, then applied just your fix and rebuilt. It neither crashed nor hung, so the original nullptr check to dodge the crash does not appear to be necessary.

I don't know if there are other cases it fixes, but it might even be worth backing it out so that if a null environment slips through again, it'll fail noisily.

[3y3]

@TheAlphaNerd , thank you for fixing this bug! I'm not pretend to be author of test.
Is there any discussions about debugger stability?
For example there is my old pr https://github.com/nodejs/node/pull/1977 with links to original issues.
Would be nice to actualize information about this bugs. I also can actualize my pr.

[MylesBorins]

The fix is now in master, you should expect this to see it in a release in the next 2 - 3 weeks on v4 and v5.

[MylesBorins]

The fix is now released in v5.6.0. Expect LTS next week

[MylesBorins]

The fix is now in lts on v4.3.1 !