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

Cookie expiration timers keep piling up in the asyncio event loop #1061

Closed
operutka opened this issue Aug 9, 2016 · 15 comments
Closed

Cookie expiration timers keep piling up in the asyncio event loop #1061

operutka opened this issue Aug 9, 2016 · 15 comments

Comments

@operutka
Copy link

operutka commented Aug 9, 2016

Hi,

I have a problem with my application since aiohttp 0.22. The application makes a lot of requests to an external REST API. It uses the aiohttp basic API and it suffers from significant slow down as the asyncio event loop gets flooded with cookie expiration timers. I know that the basic API is not effective in general as it creates a new session for every request. However, it makes no difference in my case. The external server closes connection after every request so there are no keep-alive connections and the basic API is much more comfortable in this case.

The CookieJar sets up a new expiration timer on every cookie update. In request-heavy applications, it leads to flooding the asyncio event loop with timer tasks effectively slowing down the whole process.

In my opinion, the expiration timers should be canceled on updates of existing cookies and replaced with new ones. In case of the basic API, the default CookieJar should be replaced with a dummy one to avoid scheduling the timers at all. No cookie storage is needed in case of the basic API.

Moreover, the CookieJar implementation does not need to use the timers at all. You can simply remove expired cookies when you access them.

@asvetlov
Copy link
Member

asvetlov commented Aug 9, 2016

Agree. See also #1039

Possible solutions for CookieJar:
a) Reduce a number of expiration handlers. For example if the single handler will process all cookies which will expire after a day (the most of cookies I believe) -- it may improve situation very well. Or, maybe, an hour is better threshold -- I don't know.
b) Remove cookies only on access as you've proposed.

Dummy jar may help in your case as you've mentioned.

About adding dummy jar as default implementation for basic api -- I really don't want to invest my time into the patch. Basic API is deprecated starting from 0.21 and will be removed at Jule 2017 or something like this. Now all references to basic API are removed from documentation.

Also basic API may leak resources in case of failure under certain conditions. Current ownership schema cannot avoid it and the situation cannot be improved easy. Architectural mistake produces unsolvable bugs, sorry for that.

@fafhrd91
Copy link
Member

fafhrd91 commented Aug 9, 2016

keep-alive support in server has similar problem.

what should really happen: we should have one "book-keeping" callback every 1-5 seconds. so we can remove expired cookies at once, same for keep-alive connections. that was in ticket for keep-alive

@asvetlov
Copy link
Member

asvetlov commented Aug 9, 2016

The book-keepeng sources should be in web.Application and ClientSession, not the global singleton.
Also TCPConnector is affected.

@fafhrd91
Copy link
Member

fafhrd91 commented Aug 9, 2016

it might be per TCPConnector, that should be enough

@asvetlov
Copy link
Member

asvetlov commented Aug 9, 2016

CookieJar belongs to ClientSession, not to TCPConnector. They are orthogonal concepts.

@operutka
Copy link
Author

Thanks for your reply. I'll refactor the app to use the ClientSession and a dummy jar.

@nhumrich
Copy link

nhumrich commented Aug 30, 2016

I have this same issue, except that I am not using aiohttp cookies at all. Is there a way to turn off cookies completely so expire is never called?
The issue appears to only happen if I have at least two processes running (gunicorn workers), with only a single worker, I never run into the issue

@asvetlov
Copy link
Member

Use dummy jar

@nhumrich
Copy link

@asvetlov can you give me an example? im lost in the docs trying to figure out how

@fafhrd91
Copy link
Member

this needs to be fixed before 1.0
we have same issue, so we decided to use previous stable version of aiohttp

@nhumrich
Copy link

@fafhrd91 what version are you using?

@asvetlov
Copy link
Member

0.21.6 I believe

@asvetlov asvetlov added this to the 1.0 milestone Aug 30, 2016
@danielnelson
Copy link
Contributor

Yes, when we tried to use 0.22.5, our cpu usage went to 100%. Reverting to 0.21.6 fixed the issue, did not try the dummy cookie jar code. We were also using gunicorn with multiple workers, I never tried switching to a single worker but it might explain why I didn't notice the issue during development.

@asvetlov
Copy link
Member

Fixed by #1162

@lock
Copy link

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
Development

No branches or pull requests

5 participants