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

Document streams #1150

Merged
merged 5 commits into from Sep 10, 2016
Merged

Document streams #1150

merged 5 commits into from Sep 10, 2016

Conversation

@asvetlov
Copy link
Member

@asvetlov asvetlov commented Sep 10, 2016

Explicitly document streaming API.

Fixes #1106

Also don't inherit aiohttp.StreamReader from asyncio.StreamReader.

The inheritance makes a mess.

For example asyncio.StreamReader has readuntil() method but calling it from aiohttp.StreamReader leads to crash because internal implementation differs.

@codecov-io
Copy link

@codecov-io codecov-io commented Sep 10, 2016

Current coverage is 98.30% (diff: 100%)

Merging #1150 into master will increase coverage by 0.01%

@@             master      #1150   diff @@
==========================================
  Files            28         28          
  Lines          6489       6490     +1   
  Methods           0          0          
  Messages          0          0          
  Branches       1087       1087          
==========================================
+ Hits           6378       6380     +2   
+ Misses           61         60     -1   
  Partials         50         50          

Powered by Codecov. Last update 34c3647...1eb8110

@asvetlov asvetlov merged commit e45ff8c into master Sep 10, 2016
6 checks passed
6 checks passed
codecov/patch 100% of diff hit (target 98.28%)
Details
codecov/project 98.30% (+0.01%) compared to 34c3647
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@asvetlov asvetlov deleted the document_streams branch Sep 10, 2016
@fafhrd91
Copy link
Member

@fafhrd91 fafhrd91 commented Sep 10, 2016

there was reason for inheritance. when i added inheritance asyncio did check isinstance(..., StreamReader). i don't know if it still in place

@asvetlov
Copy link
Member Author

@asvetlov asvetlov commented Sep 10, 2016

There are two checks for StreamReader in client API.
Now both are relaxed to accept asyncio.StreamReader as well as aiohttp.StreamReader explicitly as request's BODY.
I expect no backward compatibility issues.

@fafhrd91
Copy link
Member

@fafhrd91 fafhrd91 commented Sep 11, 2016

It is opossite, asyncio does not accept anything except asyncio.StreamReader

@asvetlov
Copy link
Member Author

@asvetlov asvetlov commented Sep 11, 2016

Got it.
@fafhrd91 what asyncio API call does accept asyncio.StreamReader only?
And why do we need these call?
I've grepped over asyncio source and have found nothing.

@fafhrd91
Copy link
Member

@fafhrd91 fafhrd91 commented Sep 11, 2016

asyncio still checks for StreamReader instance in streams.StreamWriter.__init__

this change will break all installations.

@asvetlov
Copy link
Member Author

@asvetlov asvetlov commented Sep 11, 2016

Yes, is has the check.
But what is your use case?
User very unlikely creates asyncio.StreamWriter manually (I mean direct writer = asyncio.StreamWriter(transport, protocol, reader, loop)) with passing aiohttp.StreamReader instance.
He uses either asyncio.open_connection() or aiohttp API.

@fafhrd91
Copy link
Member

@fafhrd91 fafhrd91 commented Sep 11, 2016

Ok, I am fine with change

Sent from my iPhone

On Sep 11, 2016, at 10:40 AM, Andrew Svetlov notifications@github.com wrote:

Yes, is has the check.
But what is your use case?
User very unlikely creates asyncio.StreamWriter manually (I mean direct writer = asyncio.StreamWriter(transport, protocol, reader, loop)) with passing aiohttp.StreamReader instance.
He uses either asyncio.open_connection() or aiohttp API.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@asvetlov
Copy link
Member Author

@asvetlov asvetlov commented Sep 11, 2016

@fafhrd91 thanks.
IIRC we've added overloaded aiohttp.StreamWriter later for solving another issue but now we really don't need to inherit from asyncio.StreamReader.

Honestly I feel the current aiohttp state (parsers-readers-writers-DataQueue) is messy.

Right now I have no proposal for fixing it but want to invest a time into.

I'm considering all mentioned parts as very deep implementation details, e.g. if we'll drop DataQueue but incorporate processing into aiohttp.StreamWriter with keeping existing functionality nothing will fail, isn't it?

P.S.
The proposed change is not a subject for next release.

@lock
Copy link

@lock lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants