Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for Error Handling #231

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Fixes for Error Handling #231

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
7912a20
fixes error: typeerror cannot read property 'body' of undefined #229
robblovell Jun 22, 2021
b343bb4
missing await #228
robblovell Jun 22, 2021
dc3b237
this timeout seems to be required for the connect to not hang and sti…
robblovell Jun 22, 2021
502edd9
increased timeout until hang can be investigated.
robblovell Jun 22, 2021
ba157db
okay, duh, return doesn't need await.
robblovell Jun 22, 2021
7a07a54
removed first timeout.
robblovell Jun 22, 2021
1125fdd
WIP error handling fixes.
robblovell Jun 22, 2021
ef53abc
spacing
robblovell Jun 22, 2021
6d82dc8
added back retries.
robblovell Jun 22, 2021
788accd
removed commented out code.
robblovell Jun 23, 2021
8709dfa
Update index.js
robblovell Jun 23, 2021
d44c80e
formatting to client.js
robblovell Jun 23, 2021
9bc5c7b
removed commented out code.
robblovell Jun 23, 2021
c03e431
formatting
robblovell Jun 23, 2021
8c9b3e2
formatting
robblovell Jun 23, 2021
e6eefe5
unnecessary retry logic.
robblovell Jun 28, 2021
72b76f7
further protection against rogue errors. Need unit test to align.
robblovell Jun 28, 2021
38799e0
Revert "further protection against rogue errors. Need unit test to al…
robblovell Jun 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ async function connectRetry(opts, retryCount = 0) {
const response = await ngrokClient.startTunnel(opts);
return response.public_url;
} catch (err) {
if (!isRetriable(err) || retryCount >= 100) {
if (!isRetriable(err) || retryCount >= 20) {
throw err;
}
await new Promise((resolve) => setTimeout(resolve, 200));
Expand Down
26 changes: 9 additions & 17 deletions src/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ class NgrokClient {
constructor(processUrl) {
this.internalApi = got.extend({
prefixUrl: processUrl,
retry: 0,
retry: 3,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, makes sense to put this back to 0.

});
}

Expand All @@ -28,20 +28,12 @@ class NgrokClient {
}
} catch (error) {
let clientError;
Copy link

@zoetrope69 zoetrope69 Jul 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

try {
const response = JSON.parse(error.response.body);
clientError = new NgrokClientError(
response.msg,
error.response,
const response = error.response ? error.response.body ? JSON.parse(error.response.body) : error.response : error;
clientError = new NgrokClientError(
error.msg,
error.details,
response
);
} catch (e) {
clientError = new NgrokClientError(
error.response.body,
error.response,
error.response.body
);
}
);
throw clientError;
}
}
Expand All @@ -51,9 +43,9 @@ class NgrokClient {
return await this.internalApi[method](path, { json: options }).then(
(response) => response.statusCode === 204
);
} catch (error) {
const response = JSON.parse(error.response.body);
throw new NgrokClientError(response.msg, error.response, response);
} catch (e) {
const response = e.response ? e.response.body ? JSON.parse(e.response.body) : e.response : e
throw new NgrokClientError(e.msg, e.response, response);
}
}

Expand Down
7 changes: 2 additions & 5 deletions src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,7 @@ function validate(opts) {
}

function isRetriable(err) {
if (!err.response) {
return false;
}
const statusCode = err.response.statusCode;
const statusCode = err.response ? err.response.statusCode : err.body? err.body.status_code : 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd put this in a separate PR personally

const body = err.body;
const notReady500 = statusCode === 500 && /panic/.test(body);
const notReady502 =
Expand All @@ -47,7 +44,7 @@ function isRetriable(err) {
statusCode === 503 &&
body.details &&
body.details.err ===
"a successful ngrok tunnel session has not yet been established";
"a successful ngrok tunnel session has not yet been established";
return notReady500 || notReady502 || notReady503;
}

Expand Down