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

Fix race conditions in the Authentication client #2635

Merged

Conversation

rdeaton-freenome
Copy link
Contributor

Related: #2626

Why are the changes needed?

The AuthorizationClient may need to pop up a webbrowser for human interaction to auth, and may do so from multiple threads and multiple grpcio channels. This manifested for us in race conditions during pyflyte register because it uses concurrent registration

cp_task_entities = list(filter(lambda x: isinstance(x, task.TaskSpec), registrable_entities))
asyncio.run(_register(cp_task_entities))

What changes were proposed in this pull request?

We block all threads and allow a single request through for authorization and cache that result for a short window to make sure that we don't need a separate request per-thread, and avoid the race condition on setting up the local HTTP server for handling the redirect.

How was this patch tested?

Lots of calls to pyflyte register in a repository with a large number of workflows.

Check all the applicable boxes

  • I updated the documentation accordingly. (docstrings, at least)
  • All new and existing tests passed.
  • All commits are signed-off.

Signed-off-by: Robert Deaton <robert.deaton@freenome.com>
@rdeaton-freenome rdeaton-freenome changed the title Fix race conditions in the Authentication cliente Fix race conditions in the Authentication client Aug 5, 2024
thomasjpfan
thomasjpfan previously approved these changes Aug 8, 2024
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

flytekit/clients/auth/auth_client.py Show resolved Hide resolved
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Copy link

codecov bot commented Aug 8, 2024

Codecov Report

Attention: Patch coverage is 25.00000% with 15 lines in your changes missing coverage. Please review.

Project coverage is 78.78%. Comparing base (286b17f) to head (aaa5828).
Report is 42 commits behind head on master.

Files Patch % Lines
flytekit/clients/auth/auth_client.py 25.00% 15 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2635      +/-   ##
==========================================
+ Coverage   76.04%   78.78%   +2.73%     
==========================================
  Files         186      187       +1     
  Lines       19081    19190     +109     
  Branches     3765     4008     +243     
==========================================
+ Hits        14510    15118     +608     
+ Misses       3914     3378     -536     
- Partials      657      694      +37     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@eapolinario eapolinario merged commit 6ababc9 into flyteorg:master Aug 15, 2024
99 of 101 checks passed
@rdeaton-freenome rdeaton-freenome deleted the rdeaton/20240801/pkce_threading branch August 16, 2024 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants