Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Add optional non blocking refresh for sync auth code #1368
feat: Add optional non blocking refresh for sync auth code #1368
Changes from 17 commits
a07551f
20f8ec4
377e19c
31e2f33
aab77bb
1848c2a
8d741d5
20941ed
3ca4b97
c7b11f8
0417312
64e1c7c
b94d6e1
88c00e1
e2623f4
b5281bd
5d30915
4b146c4
5476a8f
77ff534
a19fe0d
0e35c67
f0b218e
a6b1815
df7220c
89ceeeb
41042c2
f557447
4cecc4a
be2e2da
d941394
f5e77e5
be5f27e
fcf1f8a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
is
request
immutable or can we create a copy here?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.
Can you expand on why copy it? I don't think it is necessary, as the request should not be re-used in other places.
I think copying the request would be a breaking change.
I'd have to do some work to understand the implication of deep copying a request object.
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.
with the code above, you can just stuff the error into a field.
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 section doesn't guard access to refresh worker resources with your feature flag.
Maybe something like:
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.
Multiple requests may end up doing this refresh right?
Can we have a lock on this sync refresh?
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.
The code is using a bounded queue which should reduce duplicate work. There is a risk that there is a small amount of duplicate work, if two threads queue work at the same time. The queue itself is thread safe + has locks.
I believe using the queue to reduce duplicate work is acceptable, and that multiple threads will perform the refresh occasionally.
There should be no data corruption due to this
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 think I would prefer to remove this from the public API:
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.
can we remove this dependency and juse use TokenState.INVALID?
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.
Good point, I've made this change
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.
the base class doesn't have a _token_state field. token_state is a property derived from expiry, I think we don't need this line
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.
Great catch! I will delete this line