Skip to content

bpo-31620: asyncio.Queue leaks memory if the queue is empty and the consumers poll it frequently#3813

Merged
asvetlov merged 1 commit into
python:masterfrom
SurenNihalani:bpo-31620
Nov 7, 2017
Merged

bpo-31620: asyncio.Queue leaks memory if the queue is empty and the consumers poll it frequently#3813
asvetlov merged 1 commit into
python:masterfrom
SurenNihalani:bpo-31620

Conversation

@SurenNihalani

@SurenNihalani SurenNihalani commented Sep 29, 2017

Copy link
Copy Markdown
Contributor

@the-knights-who-say-ni

Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

@SurenNihalani SurenNihalani changed the title bpo-31620 asyncio.Queue leaks memory if the queue is empty and consumers poll it frequently bpo-31620: asyncio.Queue leaks memory if the queue is empty and consumers poll it frequently Sep 29, 2017
@SurenNihalani

Copy link
Copy Markdown
Contributor Author

signed the CLA. awaiting on profile update.

@1st1 1st1 requested review from 1st1 and removed request for rhettinger September 29, 2017 13:35
@SurenNihalani SurenNihalani changed the title bpo-31620: asyncio.Queue leaks memory if the queue is empty and consumers poll it frequently bpo-31620: asyncio.Queue leaks memory if the queue is empty and the consumers poll it frequently Sep 29, 2017
@SurenNihalani

Copy link
Copy Markdown
Contributor Author

My profile just updated to having the CLA but i am not sure how to trigger an update.

@SurenNihalani

Copy link
Copy Markdown
Contributor Author

asyncio.Queue.put also suffers from a similar bug. I'll fix that in a different PR once we merge this so all the corrections we make to this PR get transferred over.

Comment thread Lib/asyncio/queues.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is O(n) in the length of the _getters deque, which is a bit unfortunate. Perhaps it's fine since this deque should usually be short.

Comment thread Lib/test/test_asyncio/test_queues.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expected output is expected to come second.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That file is a bit consistent on the front. happy to change it in this line or change entire file to be consistent with that convention. just lmk.

@1st1 1st1 requested review from asvetlov and vstinner October 19, 2017 17:51

@asvetlov asvetlov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Sorry for delay.

@asvetlov

Copy link
Copy Markdown
Contributor

@1st1 do you want to merge or should I do it?
Also added backport to 3.6 label

@asvetlov asvetlov merged commit c62f0cb into python:master Nov 7, 2017
@miss-islington

Copy link
Copy Markdown
Contributor

Thanks @SurenNihalani for the PR, and @asvetlov for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 7, 2017
@bedevere-bot

Copy link
Copy Markdown

GH-4326 is a backport of this pull request to the 3.6 branch.

asvetlov pushed a commit that referenced this pull request Nov 7, 2017
embray pushed a commit to embray/cpython that referenced this pull request Nov 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants