Implemented readuntil in StreamResponse #4734

WisdomPill · 2020-05-11T08:14:32Z

What do these changes do?

Added readuntil method, heavily mimicking asyncio.streams and recycling the code of readline in current StreamResponse class. With this change readline uses readuntil with \n as a separator
Same for the tests that just use different separators.

Are there changes in behavior for the user?

New way to wait for new data

Related issue number

#4054

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt This change is inside another PR I submitted AioHTTPTestCase more async friendly #4732
Add a new news fragment into the CHANGES folder
- name it <issue_id>.<type> for example (588.bugfix)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

WisdomPill · 2020-05-11T08:27:42Z

I need to check if documentation work is needed or if it is automatic, meanwhile if somebody can clear my mind about this before I check it is welcome

socketpair · 2020-09-21T15:50:51Z

aiohttp/streams.py

        not_enough = True

        while not_enough:
            while self._buffer and not_enough:
                offset = self._buffer_offset
-                ichar = self._buffer[0].find(b'\n', offset) + 1
-                # Read from current offset to found b'\n' or to the end.
+                ichar = self._buffer[0].find(separator, offset) + 1


asyncio has much more effective solution. Why not to copy-paste it ?

I did not want to change much the code.

What do you think should be copied from asyncio implementation?

I think sync the implementation with asyncio would be great!
asyncio doesn't incrementally copy data to the buffer in a loop but calculates boundaries and move bytes once.

Okay, I will fix it

codecov-io · 2020-10-21T07:10:06Z

Codecov Report

Merging #4734 into master will decrease coverage by 0.02%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #4734      +/-   ##
==========================================
- Coverage   97.56%   97.54%   -0.03%     
==========================================
  Files          43       43              
  Lines        8779     8784       +5     
  Branches     1412     1413       +1     
==========================================
+ Hits         8565     8568       +3     
- Misses        100      101       +1     
- Partials      114      115       +1

Flag	Coverage Δ
unit	`97.54% <85.71%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiohttp/streams.py	`97.43% <85.71%> (-0.49%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 488b375...e94b827. Read the comment docs.

asvetlov

I can live with not the-fast-possible implementation but the documentation update is required for merging.

Please update docs/streams.rst

WisdomPill · 2020-10-22T14:45:33Z

Yes, okay, this weekend I will have some time to fix it

WisdomPill · 2020-11-01T12:07:46Z

Hello,

I tried to mimic more the asyncio implementation but it appears to me that it is quite difficult task and a lot of code will be changed.

Simply copying the data from self._buffer using boundaries is not possible as self._buffer is a deque.
To change this I can try by making self._buffer a bytearray which seems to me that it will lead to an improvement in performance but will need a lot of code rewrite.

I added the docs by the way.

WisdomPill · 2020-11-02T22:03:50Z

@asvetlov do you think the StreamReader could be written in a better and more performant way inheriting StreamReader from asyncio?

docs/streams.rst

asvetlov · 2020-11-02T22:31:54Z

Thanks for the patience!

Co-authored-by: Andrew Svetlov <[email protected]>

github-actions · 2020-11-02T22:43:21Z

💚 Backport successful

The PR was backported to the following branches:

✅ 3.8

Co-authored-by: Andrew Svetlov <[email protected]> Co-authored-by: Anas <[email protected]> Co-authored-by: Andrew Svetlov <[email protected]>

Co-authored-by: Andrew Svetlov <[email protected]>

WisdomPill requested a review from asvetlov as a code owner May 11, 2020 08:14

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label May 11, 2020

abeeeshek mentioned this pull request Sep 21, 2020

Implement StreamReader.readuntil() method #4054

Closed

socketpair reviewed Sep 21, 2020

View reviewed changes

WisdomPill added the backport-3.7 label Oct 21, 2020

asvetlov requested changes Oct 22, 2020

View reviewed changes

asvetlov added backport-3.8 and removed backport-3.7 labels Oct 25, 2020

WisdomPill added 3 commits November 1, 2020 10:08

Implemented readuntil in StreamResponse

538b3e7

Added reference to aio-libs#4054 feature in CHANGES

18e57c2

Added docs for StreamReader.readuntil

d483e2f

WisdomPill force-pushed the read_until branch from 37e4f2d to d483e2f Compare November 1, 2020 12:07

WisdomPill requested a review from webknjaz as a code owner November 1, 2020 12:07

WisdomPill added 2 commits November 1, 2020 18:27

Ran black for test/test_streams.py

99e96c2

Ran black for aiohttp/streams.py

c862fa1

asvetlov self-assigned this Nov 2, 2020

Merge branch 'master' into read_until

ace6a36

asvetlov reviewed Nov 2, 2020

View reviewed changes

docs/streams.rst Outdated Show resolved Hide resolved

Update docs/streams.rst

a6a3b45

asvetlov reviewed Nov 2, 2020

View reviewed changes

docs/streams.rst Outdated Show resolved Hide resolved

Update docs/streams.rst

e94b827

asvetlov approved these changes Nov 2, 2020

View reviewed changes

asvetlov merged commit db1a2cd into aio-libs:master Nov 2, 2020

github-actions bot pushed a commit that referenced this pull request Nov 2, 2020

Implemented readuntil in StreamResponse (#4734)

c6e83e4

Co-authored-by: Andrew Svetlov <[email protected]>

github-actions bot mentioned this pull request Nov 2, 2020

[3.8] Implemented readuntil in StreamResponse (#4734) #5196

Merged

asvetlov added a commit that referenced this pull request Nov 2, 2020

Implemented readuntil in StreamResponse (#4734) (#5196)

8ce3013

Co-authored-by: Andrew Svetlov <[email protected]> Co-authored-by: Anas <[email protected]> Co-authored-by: Andrew Svetlov <[email protected]>

commonism pushed a commit to commonism/aiohttp that referenced this pull request Apr 27, 2021

Implemented readuntil in StreamResponse (aio-libs#4734)

55c4596

Co-authored-by: Andrew Svetlov <[email protected]>

commonism pushed a commit to commonism/aiohttp that referenced this pull request Apr 27, 2021

Implemented readuntil in StreamResponse (aio-libs#4734)

bf4b84d

Co-authored-by: Andrew Svetlov <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented readuntil in StreamResponse #4734

Implemented readuntil in StreamResponse #4734

WisdomPill commented May 11, 2020 •

edited

Loading

WisdomPill commented May 11, 2020

socketpair Sep 21, 2020

WisdomPill Oct 9, 2020

asvetlov Oct 21, 2020

WisdomPill Oct 22, 2020

codecov-io commented Oct 21, 2020 •

edited by codecov bot

Loading

asvetlov left a comment •

edited

Loading

WisdomPill commented Oct 22, 2020

WisdomPill commented Nov 1, 2020

WisdomPill commented Nov 2, 2020

asvetlov commented Nov 2, 2020

github-actions bot commented Nov 2, 2020

Implemented readuntil in StreamResponse #4734

Implemented readuntil in StreamResponse #4734

Conversation

WisdomPill commented May 11, 2020 • edited Loading

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

WisdomPill commented May 11, 2020

socketpair Sep 21, 2020

Choose a reason for hiding this comment

WisdomPill Oct 9, 2020

Choose a reason for hiding this comment

asvetlov Oct 21, 2020

Choose a reason for hiding this comment

WisdomPill Oct 22, 2020

Choose a reason for hiding this comment

codecov-io commented Oct 21, 2020 • edited by codecov bot Loading

Codecov Report

asvetlov left a comment • edited Loading

Choose a reason for hiding this comment

WisdomPill commented Oct 22, 2020

WisdomPill commented Nov 1, 2020

WisdomPill commented Nov 2, 2020

asvetlov commented Nov 2, 2020

github-actions bot commented Nov 2, 2020

💚 Backport successful

WisdomPill commented May 11, 2020 •

edited

Loading

codecov-io commented Oct 21, 2020 •

edited by codecov bot

Loading

asvetlov left a comment •

edited

Loading