Skip to content

Fix contextvars.Context.copy memory leak. #191

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

Closed

Conversation

hellysmile
Copy link

Hey, after switching to 3.7 I've found that our servers being leaked according to their load.
Switching to asyncio loop removes this leak

here is pseudo code which I used to reproduce leak

import asyncio
import janus
import queue as q

from pympler import muppy
from pympler import summary


all_objects1 = muppy.get_objects()


def produce(queue):
    for x in range(1000):
        print(x)
        while True:
            try:
                queue.sync_q.put(x, timeout=0.1)
                break
            except q.Full:
                continue


async def consume(queue):
    while True:

        item = await queue.async_q.get()
        if item is ...:
            await queue.async_q.put(...)

            break

        await asyncio.sleep(0.001)


import uvloop
loop = uvloop.new_event_loop()
asyncio.set_event_loop(loop)

queue = janus.Queue(3)

coros = [
    loop.run_in_executor(None, produce, queue)
    for _ in range(10)
]

workers = set()

for _ in range(3):
    worker = loop.create_task(consume(queue))
    workers.add(worker)
    worker.add_done_callback(workers.remove)

loop.run_until_complete(asyncio.gather(*coros))

loop.run_until_complete(queue.async_q.put(...))

loop.run_until_complete(asyncio.gather(*workers))

queue.close()
loop.run_until_complete(queue.wait_closed())

all_objects2 = muppy.get_objects()

sum1 = summary.summarize(all_objects1)
sum2 = summary.summarize(all_objects2)

diff = summary.get_diff(sum1, sum2)

summary.print_(diff)
import ipdb; ipdb.set_trace()
loop.close()

summary.print_ shows ~20k of unallocated contexts

This fix does remove this leak and all tests are passed

@hellysmile hellysmile changed the title Fix contextvars.Context.copy leak. Fix contextvars.Context.copy memory leak. Aug 7, 2018
@hellysmile
Copy link
Author

travis build for 042d360 failed seems due slow travis mac ssl connection and seems not related to this PR, cuz previous commit is green

Can someone restart these builds?

1st1 added a commit that referenced this pull request Aug 7, 2018
Initial patch and memleak discovery by Victor K. @hellysmile.

Fixes #191.
@1st1
Copy link
Member

1st1 commented Aug 7, 2018

Wow! Thank you so much for tracking this down!

I have a different idea how to fix this though. Instead of fixing refcount in __dealloc__ we should do that the moment we copy the context. Otherwise we can get another kind of leak if there's a cycle between the context and the task/callback handle. Hence #192. I'll close this one.

I will issue a bugfix release for 0.11.x and 0.10.x today.

@1st1 1st1 closed this Aug 7, 2018
1st1 added a commit that referenced this pull request Aug 7, 2018
Initial patch and memleak discovery by Victor K. @hellysmile.

Fixes #191.
1st1 added a commit that referenced this pull request Aug 7, 2018
Initial patch and memleak discovery by Victor K. @hellysmile.

Fixes #191.
1st1 added a commit that referenced this pull request Aug 7, 2018
Initial patch and memleak discovery by Victor K. @hellysmile.

Fixes #191.
@1st1
Copy link
Member

1st1 commented Aug 7, 2018

v0.11.2 has just been released with a fix for this. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants