body thread helpers by thehesiod · Pull Request #3205 · aio-libs/aiohttp

thehesiod · 2018-08-20T19:59:17Z

work on resolving #3201

thehesiod · 2018-08-20T20:01:36Z

I also fixed some subclass methods which didn't match the parent class parameter declarations.

aiohttp/web_response.py

kxepal · 2018-08-20T20:10:42Z

aiohttp/web_response.py

                         if coding.value == 'gzip' else -zlib.MAX_WBITS)
+            if len(self._body) > _BODY_LENGTH_THREAD_CUTOFF:
+                loop = asyncio.get_event_loop()
+                await loop.run_in_executor(None,


What if default executor will be process one? That may hurt instead of making things better.

that's already a problem in aiohttp's use of getaddrinfo

aiohttp/web_response.py

codecov-io · 2018-08-22T00:15:12Z

Codecov Report

Merging #3205 into master will decrease coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3205      +/-   ##
==========================================
- Coverage   98.01%   97.94%   -0.07%     
==========================================
  Files          44       44              
  Lines        8511     8522      +11     
  Branches     1382     1383       +1     
==========================================
+ Hits         8342     8347       +5     
- Misses         70       74       +4     
- Partials       99      101       +2

Impacted Files	Coverage Δ
aiohttp/web_response.py	`98.41% <100%> (+0.03%)`	⬆️
aiohttp/tcp_helpers.py	`90% <0%> (-6.67%)`	⬇️
aiohttp/web_fileresponse.py	`96.51% <0%> (-1.75%)`	⬇️
aiohttp/client_reqrep.py	`97.28% <0%> (-0.16%)`	⬇️

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 0c54bef...fc57bcb. Read the comment docs.

thehesiod · 2018-08-22T19:15:38Z

hrm, codecov stuck

…helpers # Conflicts: # aiohttp/web_response.py

from aio-libs#3205

thehesiod · 2018-09-25T00:54:27Z

failed due to: The job exceeded the maximum log length, and has been terminated.

another error: aiohttp.client_exceptions.ClientConnectorSSLError: Cannot connect to host 127.0.0.1:36583 ssl:None [[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: self signed certificate (_ssl.c:871)] doesn't seem like I caused these

kxepal · 2018-09-30T12:26:16Z

aiohttp/web_response.py

+            if asyncio.iscoroutinefunction(dumps):
+                text = await dumps(data)
+            elif executor_body_size is not None and \
+                    len(data) > executor_body_size:


I was curious to try your patch "at home" and found that this check actually ruins all the idea. Imagine the following data: {"users": [{"id": 1, "name": ..., ...few more 50 fields}, ..., ..about 100 more items]}. In this case len will return 1 all the time, but response data is big enough.

Instead, I was able to make async dump easily in place where I'm sure big data will happens and this async dump will be useful. On my application side it's easy to do: I know my data and how big it could be. On aiohttp side it's hard to figure out without tricky inspections which will may negotiate all the profit of this feature.

M/b try (recursive) sizeof?

sizeof will require you somehow convert objects memory usage into string length, which wouldn't be trivial. And all this complexity just to prevent users explicitly define behaviour on their side with two lines of code?

Yeah, I agree with users needing to just opt-in by themselves.
As for converting to string length, it's not needed, just bytes it occupies in memory would be enough :)

hah! wow that was a huge oversight, thanks...however thinking about this, I still need something like a sizeof, I don't know how large the object is without doing the dump ;) I'll have to think about this for a bit

removing this from this PR, thanks guys and sorry for the distraction...however it remains a problem to be solved. Given a dict, to programmatically determine if it should be dumped in a thread or not. I suppose you just need to decide if you always want to run it in a thread or not.

No worries :)
One of the solutions might be recursively accumulating the size of stuff in the nested object with interruption of the process once reached some threshold as a flag for compression being needed as an optimization.

asvetlov · 2018-09-30T16:51:53Z

Please merge with master.
I've reduced output log size (I hope) by disabling pytest-sugar plugin.
The plugin is pretty nice, I'm still using it locally but it writes to many terminal control symbols to make the output colorized and interactive.

webknjaz · 2018-10-01T11:56:51Z

@asvetlov sugar is not the issue. The issue is that you run a lot of tests, which I'm trying to address for a long time now. You should be running test suite in just one mode per job, it'd help dramatically. That's one of the reasons of me pushing towards using tox or smth similar to unify testing layer, maybe parallelise it + possibly distribute across CIs.

webknjaz · 2018-10-01T12:01:14Z

@thehesiod this PR needs conflict resolution (rebase or merge)

…helpers # Conflicts: # tests/test_web_response.py

…helpers

thehesiod · 2018-10-08T19:37:36Z

ok I think I fixed it, followed: https://stackoverflow.com/a/23668025. Not sure how that happened as I never touched the submodules

webknjaz · 2018-10-08T19:58:21Z

aiohttp/web_response.py

-                compressobj.flush()
+            if self._zlib_thread_size is not None and \
+                    len(self._body) > self._zlib_thread_size:
+                await asyncio.get_event_loop().run_in_executor(


Wouldn't it be better to have a thread pool in place?

I think it would. If someone will set default executor as mutiprocessing one he will get very surprised by aiohttp behaviour. However, inplace thread pool will cause DoS situation - you need to limit overall threads by some sane number.

Multiprocessing pool as default is deprecated by asyncio: it doesn't work well with API like loop.getaddrinfo() anyway.

Perhaps ability to pass in thread pool? This can get complicated later if you want to support multiple pools

Make sense. Feel free to update the PR.

@asvetlov added

asvetlov · 2018-11-25T17:50:57Z

Sorry, I broke unit tests trying to resolve merge conflicts.
@thehesiod would you take a look?

asvetlov

Thanks, very close to finish.

Please add versionadded directives in doc.

Adding a test with passing explicit executor would be nice to have also.

asvetlov · 2018-11-30T12:04:58Z

docs/web_reference.rst


+   :param int zlib_executor_size: length in bytes which will trigger zlib compression
+                            of body to happen in an executor
+


Please insert .. versionadded:: 3.5 directive here.

aiohttp 3.5.0 was not released yet, only 3.5.0 alpha 0.
So please use just 3.5, the last zero should be omitted.

docs/web_reference.rst

asvetlov · 2018-11-30T17:43:28Z

docs/web_reference.rst


+   :param int zlib_executor_size: length in bytes which will trigger zlib compression
+                            of body to happen in an executor
+


aiohttp 3.5.0 was not released yet, only 3.5.0 alpha 0.
So please use just 3.5, the last zero should be omitted.

asvetlov · 2018-11-30T17:43:57Z

docs/web_reference.rst

+        .. versionadded:: 3.5.1

   :param int zlib_executor: executor to use for zlib compression
+        .. versionadded:: 3.5.1


3.5 as above.

asvetlov

Perfect!
Thank you for the patience!

thehesiod · 2018-11-30T21:50:57Z

ty guys for the help!

lock · 2019-11-30T22:05:26Z

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.

thehesiod added 3 commits August 20, 2018 12:35

Merge remote-tracking branch 'aio-libs/master'

bc42974

ensure large body prep processes run in a worker thread

fda17d8

add feature file

bf156dc

flake fix

4e6d0be

kxepal reviewed Aug 20, 2018

View reviewed changes

thehesiod added 3 commits August 20, 2018 13:19

fix based on review and add tests

1de3235

fix tests and changes based on review

b921a22

switch to new attribute

a8b570d

thehesiod commented Aug 20, 2018

View reviewed changes

aiohttp/web_response.py Outdated Show resolved Hide resolved

thehesiod added 2 commits August 20, 2018 15:14

fix and add test

1474167

bugfixes

391f995

webknjaz requested a review from asvetlov August 21, 2018 14:27

webknjaz assigned thehesiod Aug 21, 2018

thehesiod added 2 commits August 21, 2018 13:25

fix warning

1f421e7

fix import order

56d8235

thehesiod commented Aug 21, 2018

View reviewed changes

aiohttp/web_response.py Show resolved Hide resolved

fix unittests

32c8171

thehesiod added 2 commits August 21, 2018 18:18

fix coverage by fixing tests

82aac11

add another sub-test

82b3119

Merge remote-tracking branch 'aio-libs/master' into thehesiod-thread-…

ab71cea

…helpers # Conflicts: # aiohttp/web_response.py

thehesiod added a commit to thehesiod-forks/aiohttp that referenced this pull request Sep 24, 2018

add thread helper

c81e3e0

from aio-libs#3205

kxepal reviewed Sep 30, 2018

View reviewed changes

Merge remote-tracking branch 'aio-libs/master' into thehesiod-thread-…

d209ddb

…helpers # Conflicts: # tests/test_web_response.py

thehesiod added 2 commits October 8, 2018 12:31

Merge remote-tracking branch 'aio-libs/master' into thehesiod-thread-…

2f50597

…helpers

revert change?

1a501dd

reword

58b6f27

webknjaz reviewed Oct 8, 2018

View reviewed changes

asvetlov mentioned this pull request Nov 8, 2018

aiohttp 3.5 release #3384

Closed

asvetlov added 5 commits November 24, 2018 02:49

Merge branch 'master' into thehesiod-thread-helpers

2ce23cb

Update web_response.py

a0cf83b

Update web_response.py

33a4818

Merge branch 'master' into thehesiod-thread-helpers

9f31a81

Update web_response.py

274c8bc

thehesiod added 5 commits November 26, 2018 12:23

fix merge, rename param and add zlib_executor param

781ec52

fixes

b18bbe4

mypy fixes

eb8f03e

fix import order

a4a09a1

merge fix

3ae1854

asvetlov requested changes Nov 30, 2018

View reviewed changes

feedback from review

6799795

asvetlov requested changes Nov 30, 2018

View reviewed changes

feedback from review

fc57bcb

asvetlov approved these changes Nov 30, 2018

View reviewed changes

asvetlov merged commit 8c5baa3 into aio-libs:master Nov 30, 2018

thehesiod deleted the thehesiod-thread-helpers branch November 30, 2018 21:34

lock bot added the outdated label Nov 30, 2019

lock bot locked as resolved and limited conversation to collaborators Nov 30, 2019

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Nov 30, 2019


		:param int zlib_executor_size: length in bytes which will trigger zlib compression
		of body to happen in an executor

Uh oh!

Conversation

thehesiod commented Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thehesiod commented Aug 20, 2018

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-io commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thehesiod commented Aug 22, 2018

Uh oh!

thehesiod commented Sep 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asvetlov commented Sep 30, 2018

Uh oh!

webknjaz commented Oct 1, 2018

Uh oh!

webknjaz commented Oct 1, 2018

Uh oh!

thehesiod commented Oct 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asvetlov commented Nov 25, 2018

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

thehesiod commented Nov 30, 2018

Uh oh!

lock bot commented Nov 30, 2019

Uh oh!

thehesiod commented Aug 20, 2018 •

edited

Loading

codecov-io commented Aug 22, 2018 •

edited

Loading

thehesiod commented Sep 25, 2018 •

edited

Loading