Fixes for Error Handling

[robblovell]

Errors in error handing appear to have broken the connect/retry logic. I have added some defensive measures that keep the error handling from crashing and now connect is more reliable.

fix #229
mightfix #222

[philnash]

Thanks for the work on this! I've just left a couple of questions about why you chose to do a couple of things.

[philnash]

What was the situation in which there was no err.response but there was an err.body.status_code? Has that come from the changes you made in NgrokClient?

[robblovell]

From what I can tell, this doesn't come from changes to the NgrokClient. This change was made because I was receiving errors in the error handling that should have gone through the retry logic but didn't because of this problem.

[robblovell]

Given that one of the unit tests dealing with paid plans is broken now, it looks like this needs further investigation.

[robblovell]

The problem occurs when someone has a paid plan and it's in use on another machine. In other words, a tunnel cannot be created because only one tunnel is allowed.

[zoetrope69]

I'd put this in a separate PR personally

[philnash]

With retries from got and retries based on the isRetriable function we have two different ways that retries might happen. Is there a reason to include retries from got specifically?

[robblovell]

Okay, makes sense to put this back to 0.

[philnash]

It seems that one of the tests is failing too.

[robblovell]

Yes, one test fails, I make a note of that in one of the comments. It think this needs further investigation. 👍

Given that one of the unit tests dealing with paid plans is broken now, it looks like this needs further investigation.

[philnash]

Do you reckon you can write a test for when the error handling was failing before, the error from #229? I am not sure what that that situation is, so it would help me understand that this fixes that, but also hopefully help me to fix the failing test under this pull request.

[zoetrope69]

I'm also hitting these errors @robblovell on a paid plan, let me know if you need help debugging together.

[philnash]

If someone can provide a way to reproduce the error, I'd be happy to look into what is going on here too. I've just not seen this yet myself.

[zoetrope69]

heres how I was encountering the problem:

main.js

const ngrok = require("ngrok");
// ....
const  ngrokUrl = await ngrok.connect({
  addr: PORT,
  authtoken: NGROK_AUTH_TOKEN,
  region: "eu",
  subdomain: NGROK_SUBDOMAIN,
});

note: i have a paid account

package-json

  "start": "nodemon main.js"

First time running Ngrok does what I expect but then when nodemon refreshes because of a code change in main.js it errors.

[philnash]

Thanks for that @zaccolley. I will try to reproduce that myself. I can suggest a workaround though, rather than have ngrok restart when nodemon restarts you can set up your project to start ngrok once and then have nodemon restart just the app and not the tunnel. Even once this is fixed, that is likely a better way to use the two modules together.

[zoetrope69]

... start ngrok once and then have nodemon restart just the app and not the tunnel.

That looks like the best way yeah, maybe it's a bit much to expect this library handle such quick closing of ngrok and recreating

[philnash]

My guess right now, until I get some time for this, is that there's something going wrong with closing the process such that when it is started quickly again it is not ready and causes these unexpected errors.

I do think that avoiding stopping and starting the process is likely better anyway though.

[zoetrope69]

I thought the same, ngrok.kill and ngrok.disconnect don't seem to work in this scenario. Which is something I tried, I had it so it would try kill the process, wait and connect again but this didn't work either.

[zoetrope69]

I wonder if it's cause when we do a nodemon refresh we lose track of the current running ngrok process, and ngrok.kill is not going to do anything for that process at that point.

[philnash]

My theory is that we need to listen for signals that get sent and kill the ngrok child process there and not during the "exit" event. The Node docs say that you cannot run asynchronous functions for the "exit" event.

By default nodemon sends SIGUSR2, it is probably right to try to close the process for SIGINT (ctrl + c), SIGTERM and SIGHUP (see explanations of Signal codes here) as well as SIGUSR2.

Being able to handle the signals and shutdown gracefully should mean that the processes don't get in a weird way and cause unexpected errors.

[philnash]

I think things might be simpler than having to catch all the different signals. On the exit event this module was trying to run asynchronous code, but that's not supported in the exit event. However, killing a process is a synchronous command, so replacing the internal killProcess with just activeProcess.kill() should properly close down the ngrok subprocess and avoid issues when nodemon is restarting the application. This update has been made as #240.

[zoetrope69]

Nice Phil, I think we should still fix the error handling in this PR as there could be another error that isn't caught in the future?

[zoetrope69]

Couple of things here:

1. Now that we're not doing a try ... catch we don't need to create the variable at the start.
2. I think it is still worth catching errors from JSON.parse should the response body be incorrect JSON
3. I think the double nested ternary statement makes it hard to follow the logic
4. We repeat some logic that we can put in a helper function

Could we do something like this:

// .. defined above

function getResponse(error) {
  if (error.response && error.response.body) {
    let response;
    try {
      response = JSON.parse(error.response.body);
    } catch (e) {
     throw new NgrokClientError(e.msg, e.details);
    }
    return response;
  }

  return error;
}

// ... then 

const response = getResponse(error);
throw new NgrokClientError(error.msg, error.details, response);

// ... and

} catch (e) {
  const response = getResponse(e);
  throw new NgrokClientError(e.msg, e.response, response);
}

[philnash]

Nice Phil, I think we should still fix the error handling in this PR as there could be another error that isn't caught in the future?

I agree, if there's an issue in the error handling we should indeed fix it. Currently I have a couple of issues with this.

1. I can't reproduce the error you're talking about, so can't test it myself.
2. This branch currently fails a test in which an error was being thrown and is now not thrown as expected.

If we can fix the broken test and introduce at least a test case that this new code passes, then I'll be happy to merge.