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

Correct the docstring and implementation of AuthMetadataPlugin #22471

Merged
merged 1 commit into from
Mar 25, 2020

Conversation

lidizheng
Copy link
Contributor

The docstring of AuthMetadataPlugin has created confusion for our end users googleapis/google-auth-library-python#351. Our current implementation spawns a dedicated thread each time the AuthMetadataPlugin is invoked. It doesn't block the caller of the RPC. I confirmed the behavior by twitching our credentials example.

Also, this PR removed the thread pool in the plugin class to be consistent with the docstring and googleapis/google-auth-library-python#467.

CC @busunkim96

@lidizheng lidizheng added kind/bug lang/Python priority/P2 release notes: yes Indicates if PR needs to be in release notes labels Mar 24, 2020
@lidizheng lidizheng requested a review from gnossen March 25, 2020 01:20
@lidizheng
Copy link
Contributor Author

@gnossen PTAL

@gnossen
Copy link
Contributor

gnossen commented Mar 25, 2020

@lidizheng This switches the __call__ method from asynchronous to synchronous. Which thread is blocked by this operation?

@lidizheng
Copy link
Contributor Author

lidizheng commented Mar 25, 2020

@gnossen The Cython spawned thread will be blocked. For sync Python, if an HTTP request will block, it have to block on a certain thread. I think it might be a viable way to ensure the RPC caller is not blocked by the plugin callback.

If the plugin callback never finished (blocked forever or not calling the supplied callback), the RPC won't be proceed due to pending metadata. But other RPC features still works, like timeout.

Copy link
Contributor

@gnossen gnossen left a comment

Choose a reason for hiding this comment

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

I see. Nice solution! 👍

@lidizheng lidizheng merged commit a687274 into grpc:master Mar 25, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug lang/Python priority/P2 release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants