Waiters refac #3568

socketpair · 2019-01-21T17:08:35Z

What do these changes do?

Are there changes in behavior for the user?

Related issue number

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
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
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."

socketpair · 2019-01-21T17:09:26Z

@asvetlov Please take a look. Please invite others to look at it thoroughly.

socketpair · 2019-01-21T17:09:44Z

@mnach Please review

socketpair · 2019-01-21T17:11:52Z

aiohttp/streams.py

+
+            if self._buffer:
+                return (self._read_nowait_chunk(-1), False)
+                # return (self._read_nowait(-1), False)


This actually right, but break one single test, since the test is wrong (based on exact implementation rather than documentation) :) Anyway both returns do not break API. It's just an optimization.

socketpair · 2019-01-21T17:16:01Z

aiohttp/streams.py

@@ -388,31 +388,29 @@ def end_http_chunk_receiving(self) -> None:
        of the data corresponds to the end of a HTTP chunk , otherwise it is
        always False.


@asvetlov I would change the API. This function is meaningless when chunked encoding is not used. So I would raise exception if this function used on non-chunked encoding (can be checked using self._http_chunk_splits is None)

asvetlov · 2019-01-21T22:49:03Z

I agree that a waiter should return None, boolean doesn't make any sense and looks confusing.
I'm worried about _wait method modification.
In my mind, we have to make streams implementation much simpler to understanding.
I've filed #3440 issue a month ago.
Skip slab allocators part when reading.

Long story short:

Introduce AbstractStreamReader. Initially, it should have all StreamReader public methods. We can reconsider dropping (with depreciation) a part of them later if needed.
Implement RawStreamReader for raw body data, ChunkedStreamReader for chunked encoding, a reader for compressed source and WebSocket etc.
All readers are based on the raw reader. Chunked reader use it as .raw attribute for example. The decompressor can be built on top of the raw reader or chunked one, pointing to the source stream be own raw attribute. And so on.

What do you think about this structure? Is it viable, did I miss something?

codecov-io · 2019-01-22T02:26:37Z

Codecov Report

Merging #3568 into master will increase coverage by 0.12%.
The diff coverage is 90.47%.

@@            Coverage Diff             @@
##           master    #3568      +/-   ##
==========================================
+ Coverage    97.9%   98.02%   +0.12%     
==========================================
  Files          43       43              
  Lines        8560     8781     +221     
  Branches     1378     1489     +111     
==========================================
+ Hits         8381     8608     +227     
+ Misses         74       71       -3     
+ Partials      105      102       -3

Impacted Files	Coverage Δ
aiohttp/streams.py	`98.7% <90.47%> (-0.02%)`	⬇️
aiohttp/connector.py	`97.68% <0%> (ø)`	⬆️
aiohttp/client_reqrep.py	`98.78% <0%> (+1.44%)`	⬆️

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 bb148e6...1ab3b1b. Read the comment docs.

This is just a simplifaction. Actually, the value means nothing.

socketpair · 2019-01-22T15:26:39Z

@asvetlov I have fixed everything. Merge ?

socketpair · 2019-01-24T03:33:50Z

Merge ?

socketpair · 2019-01-27T10:13:30Z

PING

asvetlov · 2019-01-27T15:54:55Z

Hi.
Sorry, was terrible busy last week.
Will check the PR in a few hour.
I have a feeling that the PR worth a new 3.5 release, isn't it?

socketpair · 2019-01-27T17:10:52Z

@asvetlov regarding 3.5 release: it does not add any new features or fixes any bugs. Since I don't know rules for LTS releases, I can not answer.

socketpair · 2019-01-29T16:43:00Z

OK, I'm sure changes are correct, so please merge.

asvetlov · 2019-01-29T16:53:25Z

Thank you and sorry for waiting

* 🐽 streams: change waiters return value to None This is just a simplifaction. Actually, the value means nothing. * 🐽 stream.readchunk() refactoring (cherry picked from commit 36331ce) Co-authored-by: Коренберг Марк <[email protected]>

socketpair requested a review from asvetlov as a code owner January 21, 2019 17:08

socketpair commented Jan 21, 2019

View reviewed changes

socketpair force-pushed the waiters_refac branch 2 times, most recently from 3947b19 to 0fc8f4c Compare January 21, 2019 18:39

socketpair requested a review from webknjaz as a code owner January 21, 2019 18:39

socketpair force-pushed the waiters_refac branch 2 times, most recently from 5428328 to e931535 Compare January 21, 2019 18:47

socketpair force-pushed the waiters_refac branch 3 times, most recently from 5bfcb37 to 42cd03c Compare January 22, 2019 10:13

socketpair added 2 commits January 22, 2019 18:16

🐽 streams: change waiters return value to None

4476773

This is just a simplifaction. Actually, the value means nothing.

🐽 stream.readchunk() refactoring

1ab3b1b

socketpair force-pushed the waiters_refac branch from 42cd03c to 1ab3b1b Compare January 22, 2019 13:16

asvetlov approved these changes Jan 29, 2019

View reviewed changes

asvetlov merged commit 36331ce into master Jan 29, 2019

asvetlov deleted the waiters_refac branch January 29, 2019 16:53

asvetlov mentioned this pull request Jan 30, 2019

aiohttp 3.5 support #3588

Closed

lock bot added the outdated label Jan 30, 2020

lock bot locked as resolved and limited conversation to collaborators Jan 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Waiters refac #3568

Waiters refac #3568

socketpair commented Jan 21, 2019 •

edited

Loading

socketpair commented Jan 21, 2019

socketpair commented Jan 21, 2019

socketpair Jan 21, 2019 •

edited

Loading

socketpair Jan 21, 2019 •

edited

Loading

asvetlov commented Jan 21, 2019

codecov-io commented Jan 22, 2019 •

edited

Loading

socketpair commented Jan 22, 2019 •

edited

Loading

socketpair commented Jan 24, 2019

socketpair commented Jan 27, 2019

asvetlov commented Jan 27, 2019

socketpair commented Jan 27, 2019

socketpair commented Jan 29, 2019

asvetlov commented Jan 29, 2019

		@@ -388,31 +388,29 @@ def end_http_chunk_receiving(self) -> None:
		of the data corresponds to the end of a HTTP chunk , otherwise it is
		always False.

Waiters refac #3568

Waiters refac #3568

Conversation

socketpair commented Jan 21, 2019 • edited Loading

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

socketpair commented Jan 21, 2019

socketpair commented Jan 21, 2019

socketpair Jan 21, 2019 • edited Loading

Choose a reason for hiding this comment

socketpair Jan 21, 2019 • edited Loading

Choose a reason for hiding this comment

asvetlov commented Jan 21, 2019

codecov-io commented Jan 22, 2019 • edited Loading

Codecov Report

socketpair commented Jan 22, 2019 • edited Loading

socketpair commented Jan 24, 2019

socketpair commented Jan 27, 2019

asvetlov commented Jan 27, 2019

socketpair commented Jan 27, 2019

socketpair commented Jan 29, 2019

asvetlov commented Jan 29, 2019

socketpair commented Jan 21, 2019 •

edited

Loading

socketpair Jan 21, 2019 •

edited

Loading

socketpair Jan 21, 2019 •

edited

Loading

codecov-io commented Jan 22, 2019 •

edited

Loading

socketpair commented Jan 22, 2019 •

edited

Loading