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

Possible token refresh race condition with streams #292

Open
C0urante opened this issue Jun 15, 2019 · 1 comment
Open

Possible token refresh race condition with streams #292

C0urante opened this issue Jun 15, 2019 · 1 comment

Comments

@C0urante
Copy link

C0urante commented Jun 15, 2019

Hi @mattbdean, just want to start by saying I really enjoy the library you've written here and it's saved me a lot of work for my own project, a Reddit source connector for Kafka. Thanks man!

So, the issue--like the title mentions, I believe there's a race condition with token refresh. It may or may not be specific to streams, but that's the only place I've noticed it so far.

In the RedditClient.request(...) method, a check for token refresh is made based on AuthManager.needsRenewing(); if and only if that method returns true, the token is refreshed. However, since the needsRenewing method only returns whether or not the token has already expired and not whether it's likely to expire soon, it's possible that in between that check and the actual request to Reddit, the token expires.

I think one easy fix could be to alter the needsRenewing method to return whether a given percentage of the token's lifetime has elapsed; this could be anything from 50% to 99%, depending on how expensive it is to fetch a new token and how much time is expected to elapse between calls to that method and requests that then use the possibly-expired token. If you agree, I'd be happy to open a PR up to add this--let me know.

@C0urante
Copy link
Author

It should also be noted that a quick workaround I've discovered is to silently swallow ApiExceptions whose status code is 401 and immediately retry, so this isn't a super urgent or catastrophic problem. However, it still seems worth addressing in the JRAW library itself to avoid requiring this workaround if possible.

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

No branches or pull requests

1 participant