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

Resolve unauthorized error every hour #184

Closed
wants to merge 1 commit into from
Closed

Conversation

pallavi2209
Copy link
Contributor

Before:
One generator results in 401 error and sets token to null to retrieve fresh token
By the time the first generator gets the token, other generators also result in 401 error, and create error samples (they assume they have right token and it is some legit 401 error)

Now:
Few generator clones assume they have right token (until first gen sets the token to null) -> results in 401 -> go and get new token -> try again
Once the first gen sets teh token to null -> Other generators can recognize that there is some generator on its way to get the token and they wait for first generator to get the token.

@pallavi2209 pallavi2209 requested review from iamigo and jgraff2 May 8, 2019 23:27
@coveralls
Copy link

Coverage Status

Coverage decreased (-2.4%) to 91.4% when pulling 11c95da on handle-new-token into f3da025 on master.

@iamigo
Copy link
Contributor

iamigo commented May 9, 2019

@pallavi2209 tests?

*/

const startTime = new Date().getTime();
const tokenWaitInterval = 100 || Number(process.env.TOKEN_WAIT_INTERVAL);
Copy link
Contributor

Choose a reason for hiding this comment

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

it will never look up the value from process.env because 100 is always truthy
i think you meant
Number(process.env.TOKEN_WAIT_INTERVAL) || 100;

@pallavi2209
Copy link
Contributor Author

Let me try and add some tests.

@jgraff2
Copy link
Contributor

jgraff2 commented May 9, 2019

I think this is getting too complicated for a short-term fix. If this level of complexity is required to make it work, we should just address the root cause now. I created a story for that yesterday.
@pallavi2209 @iamigo

@iamigo
Copy link
Contributor

iamigo commented May 9, 2019

@jgraff2 thx for your feedback on this

now that we have a manageable immediate-term change in place, I'm all for doing it right

@pallavi2209
Copy link
Contributor Author

@jgraff2 @iamigo I totally agree - its not worth spending effort here if we have to change it anyway. Let me close this PR and we can prioritize your story as soon as we can.

@pallavi2209 pallavi2209 closed this May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants