Context managed transports #1218
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: Florimond Manca <florimond.manca@gmail.com>
Co-authored-by: Florimond Manca <florimond.manca@gmail.com>
Co-authored-by: Florimond Manca <florimond.manca@gmail.com>
Co-authored-by: Florimond Manca <florimond.manca@gmail.com>
|
@florimondmanca Your reviews are always on-point. Wonderful, thank you. ✨🔥✨ |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #1145
Note that once #977 is addressed we'll be able to test the proxies case more elegantly.
We're not being clever here about "ensure other transports still close if an exceptions occurs during
__exit__in one transport". (Or the other fiddly case, of ensuring the__exit__is called on any already__enter__'d transports, if an exception occurs during__enter__.)I think we should probably treat that as a follow up, and it's possible that we might only want to do so once #977 is addressed.
I think thats probably an okay approach to take since it's not any worse than our existing behaviour, which anyway doesn't handle "ensure other transports still close if an exceptions occurs during
closein one transport", and besides exceptions during transport__enter__/__exit__are pretty much unrecoverable issues, and it's not really any worse if they happen to potentially leave other transports hanging open.(Actually having talked through it I'm not event convinced that we should bother handling that case at all, but either way let's talk it through as a follow-up)