- Notifications
You must be signed in to change notification settings - Fork1k
chore: remove duplicate validate calls on same oauth token#11598
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| ifoptions.NewTicker==nil { | ||
| options.NewTicker=func(duration time.Duration) (tick<-chan time.Time,donefunc()) { | ||
| ticker:=time.NewTicker(duration) | ||
| returnticker.C,ticker.Stop | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is OK for now but we should probably swap this out for something better later on 🕰️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree. Some fake clock solution would be ideal, then we can use across all tests more generally.
Uh oh!
There was an error while loading.Please reload this page.
What this does
If we have an invalid token in the db, we should not spam github with the same validate call over and over.
I am not sure how often this was hit in production, since most keys that go invalid expire, which we already check for.
This happens if someone manually revokes a token.
Related to#10853
Thoughts
I think this should really be solved more generally. We should remember when a token is invalid, and not try again. The browser and other things also trigger this call, which we could deduplicate with a cache.
This would save some calls in all cases.
The metrics indicate we are potentially sending more calls from TokenSource thenValidate though.