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: #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: #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: #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 25776f3 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.