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
Expose methods for closing async credential transport sessions #9090
Expose methods for closing async credential transport sessions #9090
Changes from 16 commits
b4dacdd
812ce4f
e4779da
dea61f8
c805092
6f7b85c
3bb53ca
d70940d
4618c97
87fcd36
6f34be0
ab12fb9
6cd4eb9
fcabaa6
58d1b2e
231bed9
a3cb1a4
5330c31
6e206e2
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.
I have a concern here: do we expect customer to "async enter" all the credentials in the chain, or should we have a aenter here that loop thourgh all of them and enter them?
Can I see a sample of usage of this one?
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 what I want to enable:
I think
close
is the important API. I don't expect anyone to "enter" or "open" a credential. I haveaenter
doing nothing here because the credential doesn't know which members of its chain will send requests, and transports will open sessions as needed. (At least, our current transport implementations will. Perhaps we should make explicit who's expected to open a transport.)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.
If you implement
__aenter__
/__aexit__
(e.g. you are an async context manager), then you are strongly signalling to users that usingasync with
is general goodness, but you can do the closing yourself if you so see fit.If we don't want to give an example of intended use with
async with
, then why is it an async context manager?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.
To be consistent with everything else in the SDK that wraps async transport by exposing
__aenter__
,__aexit__
, andclose
. This PR does add anasync with
example to the README, and it's okay to use credentials that way. Every other credential's__aenter__
invokes its transport's__aenter__
.The awkwardness for
ChainedTokenCredential.__aenter__
is that if it opens sessions for N credentials, N-1 of them may never be used, at some cost dependent on the HTTP client's implementation. These sessions will all be closed by__aexit__
, but I thought it unnecessary to open them given that our async transports will do so as needed.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 getting a bit more odd the more methods you add to the class. I know I asked before, but I can't remember the answer - why is this a class with a
__new__
and not just a function that returns the correct credential type?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.
To hide the implementations while providing the right surface for documentation and code completion. It could instead hold an instance of the appropriate credential and wrap its methods... #9302