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.

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

Stop network error trading a token request for a token failing the connection #419

Merged
merged 1 commit into from
Oct 11, 2017

Conversation

SimonWoolf
Copy link
Member

c.f. customer report at https://app.intercom.io/a/apps/ua39m1ld/respond/inbox/all/conversations/12117963267

What's happened is that you've run into an interesting bug, because of running an auth server locally. If the library can't obtain a token request due to network conditions, it will go to disconnected , as expected. If it obtains a token but can't connect, similarly. But if it obtains a token request fine, but can't exchange that for a token, then the bug is triggered it goes into failed.

The reason this hasn't been reported before is because it's a condition that basically only happens if you're running an auth server locally -- it production, it's never the case that a client's internet connection is down but it can still talk to the auth server.

@paddybyers
Copy link
Member

LGTM

@@ -437,6 +437,11 @@ var Auth = (function() {
tokenRequest(tokenRequestOrDetails, function(err, tokenResponse, headers, unpacked) {
if(err) {
Logger.logAction(Logger.LOG_ERROR, 'Auth.requestToken()', 'token request API call returned error; err = ' + Utils.inspectError(err));
if(!(err && err.code)) {
Copy link
Member

@mattheworiordan mattheworiordan Oct 10, 2017

Choose a reason for hiding this comment

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

I find this logic odd to read, wouldn't if(err && !err.code) make it simpler to comprehend, or have I misunderstood what this is doing?

Copy link
Member

Choose a reason for hiding this comment

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

since it's inside the if(err) check then surely we just need if(!err.code) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

good point

@SimonWoolf SimonWoolf merged commit d5bffc2 into master Oct 11, 2017
@SimonWoolf SimonWoolf deleted the network-error-failed branch October 11, 2017 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants