Fix redis connection leak with `app.send_task` #7685

wochinge · 2022-08-05T08:51:05Z

Description

This test shows that there is a Redis connection leak when using app.send_task.
The connection leak doesn't happen if task.delay() is used.

Todos:

find solution

wochinge · 2022-08-05T08:51:37Z

t/integration/test_tasks.py

+        redis = get_redis_connection()
+        connections_before = len(redis.client_list())
+
+        result = manager.app.send_task('t.integration.tasks.add', args=(1, 2))


if we use add.delay() then everything works

wochinge · 2022-08-05T08:52:12Z

t/integration/test_tasks.py

        ]
        assert result.get(timeout=3) == 3
        assert get_active_redis_channels() == []
+
+    def test_redis_connection_cleanup_with_send_task(self, manager):


@auvipy Do you have some ideas why this test would fail? Happy to do a fix but I'd need some pointers

it might be related to #6963 .

It definitely seems like that the connection always stays in the pending_unsubscribe_channels within the Redis client when using app.send_task. I tried with the solution from the linked issue but that doesn't resolve things.

@auvipy can you please give me some pointers where the issue might be?

lgtm-com · 2022-08-05T09:13:38Z

This pull request fixes 1 alert when merging 6c3378e into bdbf6d6 - view on LGTM.com

fixed alerts:

1 for Non-exception in 'except' clause

codecov · 2022-08-05T13:46:42Z

Codecov Report

Merging #7685 (6c5d575) into master (3db7c9d) will decrease coverage by 1.01%.
The diff coverage is 76.92%.

❗ Current head 6c5d575 differs from pull request most recent head b1cfd8c. Consider uploading reports for the commit b1cfd8c to get more accurate results

@@            Coverage Diff             @@
##           master    #7685      +/-   ##
==========================================
- Coverage   89.75%   88.74%   -1.02%     
==========================================
  Files         138      138              
  Lines       17000    17007       +7     
  Branches     2510     2493      -17     
==========================================
- Hits        15259    15093     -166     
- Misses       1491     1673     +182     
+ Partials      250      241       -9

Flag	Coverage Δ
unittests	`88.73% <76.92%> (-1.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
celery/backends/base.py	`92.53% <57.14%> (-0.41%)`	⬇️
celery/app/base.py	`96.45% <100.00%> (ø)`
celery/concurrency/eventlet.py	`0.00% <0.00%> (-99.15%)`	⬇️
celery/backends/asynchronous.py	`48.91% <0.00%> (-16.46%)`	⬇️
celery/__init__.py	`75.86% <0.00%> (-8.63%)`	⬇️
celery/app/autoretry.py	`90.32% <0.00%> (-3.23%)`	⬇️
celery/utils/imports.py	`87.50% <0.00%> (-2.28%)`	⬇️
celery/bin/base.py	`47.84% <0.00%> (-2.16%)`	⬇️
celery/bin/celery.py	`70.43% <0.00%> (-1.74%)`	⬇️
celery/backends/redis.py	`91.98% <0.00%> (-0.28%)`	⬇️
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

auvipy

can you please rebase this?

wochinge · 2022-08-12T14:17:49Z

done @auvipy

thedrow · 2022-08-14T07:05:51Z

t/integration/test_tasks.py

@@ -328,3 +329,16 @@ def test_asyncresult_get_cancels_subscription(self, manager):

        new_channels = [channel for channel in get_active_redis_channels() if channel not in channels_before_test]
        assert new_channels == []
+
+    def test_redis_connection_cleanup_with_send_task(self, manager):


I suggest we mark the test as xfailed and merge this PR.
There should be a matching issue for this PR so that we'll know this isn't yet fixed.

@thedrow I don't think that merging this before fixing this is helpful. There are already multiple issues pointing towards a connection leak:

Redis result consumer is not drained periodically (memory leak and possible connection reset) #6963

Redis result backend connections leak #6819

What I would I need is some pointers / help. Besides that I'd be more than happy to investigate and help with a fix.

@thedrow I don't think that merging this before fixing this is helpful. There are already multiple issues pointing towards a connection leak:

* [Redis result consumer is not drained periodically (memory leak and possible connection reset) #6963](https://github.com/celery/celery/issues/6963) * [Redis result backend connections leak #6819](https://github.com/celery/celery/issues/6819)

What I would I need is some pointers / help. Besides that I'd be more than happy to investigate and help with a fix.

that should be the spirit

wochinge · 2022-08-19T13:44:32Z

@auvipy I spent some time debugging it and that's the situation:

the local cache here doesn't work with eventlet as each greenthread has unique ID and hence its own local() instance. You can see this in the code snippet below as well in the comment here:

import threading
from time import sleep

import eventlet
eventlet.monkey_patch()


def f(idx: int):
    t = threading.current_thread()
  	  # We always have a different ID 
    print(threading.get_ident(), idx)

pool = eventlet.GreenPool(size=1)

threads = []
for i in range(10):
    t = pool.spawn(f, i)
    threads.append(t)

for t in threads:
    t.wait()

this means that with eventlets we create one backend connection per task. These caches are not always cleaned up correctly. I've added a piece of code which cleans up the cache for each task in case it's eventlet cache. Note that we can't test this as the integration tests run in prefork mode. If you want to reproduce it you need to:
- Change prefork to eventlet here
- Monkeypatch the eventlet stuff here by adding these lines:
```
import eventlet
eventlet.monkey_patch()
```
The above stuff should help with leaking connections. The other question is whether we can fix the cache for eventlet pools. I don't have a good solution for this at the moment. I'll explore using cached_property for Redis.

wochinge · 2022-08-19T15:07:19Z

Added a potential fix for the caching issues with eventlet. Any idea why the CI is failing? In my opinion a bit weird as it's failing in somewhat unrelated parts.

lgtm-com · 2022-08-19T15:53:12Z

This pull request introduces 1 alert when merging b1cfd8c into 3db7c9d - view on LGTM.com

new alerts:

1 for Unused local variable

thedrow · 2022-08-24T11:29:24Z

celery/backends/base.py

+        try:
+            from eventlet import corolocal
+            task_backend_cache = self.app.local
+            # Whenever an eventlet pool spawns a coroutine it has a different identifier and hence will acquire
+            # a new coroutine-local storage. To avoid that we leak connections we explicitly clean out the
+            # cache once the coroutine is done.
+            if isinstance(task_backend_cache, corolocal.local):
+                task_backend_cache.__dict__.clear()
+        except ImportError:
+            # Eventlet is not installed and hence not used
+            pass


I'm not sure I am content with coupling eventlet or any other concurrency mode with the backend. Is there another way to resolve this issue?

auvipy · 2023-02-07T09:36:49Z

@wochinge would you mind trying #8058?

wochinge · 2023-02-07T12:30:51Z

We're unfortunately no longer running into this (we put ignore_result whereever possible), sorry 🤷🏻

hzc989 · 2023-07-12T14:06:23Z

@wochinge would you mind trying #8058?

after upgrading to 5.3.1 , with setting result_backend_thread_safe=true, works for us.

p.s. we've try 4.4.7 & 5.2.7 , both failed with the same problem.

auvipy · 2023-07-12T15:00:28Z

thanks for the feedback Wong!

wochinge changed the title ~~redis connection leak~~ Fix redis connection leak with app.send_task Aug 5, 2022

wochinge commented Aug 5, 2022

View reviewed changes

wochinge mentioned this pull request Aug 9, 2022

Redis result backend connections leak #6819

Closed

18 tasks

auvipy requested changes Aug 12, 2022

View reviewed changes

wochinge force-pushed the redis-connection-leak branch from 6c3378e to 6455a47 Compare August 12, 2022 14:17

wochinge requested a review from auvipy August 12, 2022 14:17

thedrow requested changes Aug 14, 2022

View reviewed changes

auvipy added this to the 5.3 milestone Aug 14, 2022

add test for connection leak

9cb11e9

wochinge force-pushed the redis-connection-leak branch from 6455a47 to 9cb11e9 Compare August 19, 2022 07:58

wochinge added 2 commits August 19, 2022 16:27

fix: explicitly clear cache for eventlet pools

b6aff0a

feat: share connections among threads if backend is thread-safe

b1cfd8c

wochinge force-pushed the redis-connection-leak branch from 6c5d575 to b1cfd8c Compare August 19, 2022 14:28

thedrow reviewed Aug 24, 2022

View reviewed changes

wochinge closed this Feb 7, 2023

wochinge deleted the redis-connection-leak branch February 7, 2023 12:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix redis connection leak with `app.send_task` #7685

Fix redis connection leak with `app.send_task` #7685

wochinge commented Aug 5, 2022

wochinge Aug 5, 2022

wochinge Aug 5, 2022

wochinge Aug 5, 2022

wochinge Aug 5, 2022

wochinge Aug 12, 2022

lgtm-com bot commented Aug 5, 2022

codecov bot commented Aug 5, 2022 •

edited

auvipy left a comment

wochinge commented Aug 12, 2022

thedrow Aug 14, 2022

wochinge Aug 15, 2022

auvipy Aug 15, 2022

wochinge commented Aug 19, 2022

wochinge commented Aug 19, 2022

lgtm-com bot commented Aug 19, 2022

thedrow Aug 24, 2022

auvipy commented Feb 7, 2023

wochinge commented Feb 7, 2023

hzc989 commented Jul 12, 2023

auvipy commented Jul 12, 2023

Fix redis connection leak with app.send_task #7685

Fix redis connection leak with app.send_task #7685

Conversation

wochinge commented Aug 5, 2022

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Aug 5, 2022

codecov bot commented Aug 5, 2022 • edited

Codecov Report

auvipy left a comment

Choose a reason for hiding this comment

wochinge commented Aug 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wochinge commented Aug 19, 2022

wochinge commented Aug 19, 2022

lgtm-com bot commented Aug 19, 2022

Choose a reason for hiding this comment

auvipy commented Feb 7, 2023

wochinge commented Feb 7, 2023

hzc989 commented Jul 12, 2023

auvipy commented Jul 12, 2023

Fix redis connection leak with `app.send_task` #7685

Fix redis connection leak with `app.send_task` #7685

codecov bot commented Aug 5, 2022 •

edited