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

Check for proper method to close stream in asynchronous case #1316

Merged
merged 1 commit into from
Sep 24, 2020

Conversation

daa
Copy link
Contributor

@daa daa commented Sep 23, 2020

Asynchronous httpcore streams do not have close() method, they provide aclose(). Fixes #1315 .

@daa
Copy link
Contributor Author

daa commented Sep 23, 2020

To my mind such attribute checking is rather dubious because if stream must be closed then method to close it is part of its interface and thus all wrappers, mocks or substitutions must to implement it even if they are not required to be closed.

Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

Thanks!

@florimondmanca
Copy link
Member

To my mind such attribute checking is rather dubious

I'd tend to agree yup, since the AsyncByteStream and SyncByteStream interface, documented here: https://www.encode.io/httpcore/api/, specify a .aclose() and .close() method respectively. Happy to consider removing this attribute checking as a follow-up!

@florimondmanca florimondmanca merged commit 78afd08 into encode:master Sep 24, 2020
@florimondmanca florimondmanca mentioned this pull request Sep 24, 2020
@tomchristie
Copy link
Member

Happily we'll be moving away from the stream .close() interface anyways.

Once we're treating HTTPTransport.request() as a context manager byte streams will just be either Iterable[byte] or AsyncIterable[byte]. (See #1274, encode/httpcore#145)

@daa daa deleted the check-proper-async-close-method branch September 24, 2020 12:47
@daa
Copy link
Contributor Author

daa commented Sep 24, 2020

Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Async client does not close underlying stream thus leaving connection active which leads to hitting pool limit
3 participants