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

Using asgiref.local.Local with gevent leads to memory leaks #144

Closed
reupen opened this issue Feb 26, 2020 · 7 comments · Fixed by #152
Closed

Using asgiref.local.Local with gevent leads to memory leaks #144

reupen opened this issue Feb 26, 2020 · 7 comments · Fixed by #152

Comments

@reupen
Copy link

reupen commented Feb 26, 2020

We use Django with Gunicorn and gevent workers. Since updating to Django 3, we started seeing some reasonably severe memory leaks in our web processes.

I bisected one of the leaks I could reproduce locally to django/django@a415ce7.

In particular, it seems to be caused by the switch from threading.local to asgiref.local.Local.

I investigated a fair bit. There seem to be a few things going on:

(Also, I'm not sure how gevent-friendly CLEANUP_INTERVAL = 60 is.)

It could be argued that gevent-related bugs are a problem with gevent. However, I think this problem is quite severe and also not easily noticed as it's mostly a silent problem.

While any fixes (here or in gevent) would be appreciated, it would be good to document the incompatibility too.

(I tested gevent 1.4.0 and 1.5a3.)

Thanks

@reupen reupen changed the title asgiref.local.Local appears to be incompatible with Gevent asgiref.local.Local appears to be incompatible with gevent Feb 26, 2020
@andrewgodwin
Copy link
Member

Yeah, gevent does a lot of weird things that I don't know we'll be able to plug entirely.

Are you letting gevent monkeypatch the threading module? If you disable that, it might help with all the checks that are failing. If that is it, I don't see an issue sticking an explicit warning in there if that's what happens; not sure I want to teach Local about greenlets, though...

@reupen
Copy link
Author

reupen commented Feb 26, 2020

Are you letting gevent monkeypatch the threading module?

Yes, Gunicorn does that when using gevent workers: https://github.com/benoitc/gunicorn/blob/7d8c92f48a6d9cee6b15fbdade6981721182d073/gunicorn/workers/ggevent.py#L38

@andrewgodwin
Copy link
Member

That'll be the problem then. Hard to reason about threads when they're... not threads.

I'll need to think about the best approach to this one. I really don't want to have asgiref depend on gevent at all, but detecting that threads aren't threads is going to be tricky otherwise.

@reupen
Copy link
Author

reupen commented Feb 26, 2020

@jamadden – any input or thoughts about this?

reupen added a commit to uktrade/data-hub-api that referenced this issue Mar 11, 2020
We've been suffering from a memory leak since upgrading to Django 3 which is believed to be caused by django/asgiref#144.

This adds a workaround which is inactive by default. This is so it can be we can check that it fixes the problem in e.g. the dev environment.
reupen added a commit to uktrade/data-hub-api that referenced this issue Mar 11, 2020
We've been suffering from a problematic memory leak since upgrading to Django 3 which is believed to be caused by django/asgiref#144.

The memory leak was causing worker processes to exhaust available memory and crash and restart.

This adds a workaround which is inactive by default. This is so it can be we can verify that it fixes the problem in e.g. the dev environment.
reupen added a commit to uktrade/data-hub-api that referenced this issue Mar 11, 2020
We've been suffering from a problematic memory leak since upgrading to Django 3 which is believed to be caused by django/asgiref#144.

The memory leak is causing Gunicorn worker processes to exhaust available memory and crash and restart.

This adds a workaround which is inactive by default. This is so it can be we can verify that it fixes the problem in e.g. the dev environment.
reupen added a commit to uktrade/data-hub-api that referenced this issue Mar 12, 2020
We've been suffering from a problematic memory leak since upgrading to Django 3 which is believed to be caused by django/asgiref#144.

The memory leak is causing Gunicorn worker processes to exhaust available memory and crash and restart.

This adds a workaround which is inactive by default. This is so it can be we can verify that it fixes the problem in e.g. the dev environment.
reupen added a commit to uktrade/data-hub-api that referenced this issue Mar 12, 2020
We've been suffering from a problematic memory leak since upgrading to Django 3 which is believed to be caused by django/asgiref#144.

The memory leak is causing Gunicorn worker processes to exhaust available memory and crash and restart.

This adds a workaround which is inactive by default. This is so it can be we can verify that it fixes the problem in e.g. the dev environment.
reupen added a commit to uktrade/data-hub-api that referenced this issue Mar 12, 2020
We've been suffering from a problematic memory leak since upgrading to Django 3 which is believed to be caused by django/asgiref#144.

The memory leak is causing Gunicorn worker processes to exhaust available memory and crash and restart.

This adds a workaround which is inactive by default. This is so it can be we can verify that it fixes the problem in e.g. the dev environment.
reupen added a commit to uktrade/data-hub-api that referenced this issue Mar 12, 2020
We've been suffering from a problematic memory leak since upgrading to Django 3 which is believed to be caused by django/asgiref#144.

The memory leak is causing Gunicorn worker processes to exhaust available memory and crash and restart.

This adds a workaround which is inactive by default. This is so it can be we can verify that it fixes the problem in e.g. the dev environment.
@reupen reupen changed the title asgiref.local.Local appears to be incompatible with gevent Using asgiref.local.Local with gevent leads to memory leaks Mar 17, 2020
@andrewgodwin
Copy link
Member

OK, this might well be solved by the new patch for Local that removes directly inspecting the class type of threads (https://github.com/django/asgiref/tree/new_local)

If someone wants to try installing the new_local branch and seeing how it fares, that would be great.

@reupen
Copy link
Author

reupen commented Mar 19, 2020

I've given it a quick test locally – it does indeed seem to have fixed the problem. Thanks!

@andrewgodwin
Copy link
Member

Alright, I'll get it merged in then, as it seems to solve at least two problems and not cause any issues!

andrewgodwin added a commit that referenced this issue Mar 19, 2020
This one stores on threads/tasks directly, much like the implementation in the standard library. This removes the need for garbage-collection of local storage, and should be more compatible with libraries that override threading, like gevent.

Fixes #144.
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 a pull request may close this issue.

2 participants