Fix Redis disconnect handling

[popravich]

This fixes celery/celery#3898 issue.

As far as I understand the celery hang is happening under the following scenario:

• redis connection is created in main thread;
• then, process forks a worker and it receives copies of all open sockets
• when the worker is done (max_tasks_per_child) it cleans up its resources (closes connections)
• redis-py client closes its socket in connection,
  however before close() it calls shutdown() which shuts down all copies of socket shared
  between all the processes
  So, again, if I understand all correctly, main process is still able to read that socket and poll always
  returns it as ready however it is in unoperational state.

[wolph]

While this code does look like a nice improvement, I would vote against merging this in until the underlying hiredis bug is solved.

Since this exact same code works reliably without hiredis we can conclude that either hiredis or the hiredis wrapper code is the actual culprit.

[auvipy]

Is there any possible way to test this proposed change?

[seocam]

Is there any possible way to test this proposed change?

I suppose you are talking about a test to avoid regression but I have this running on my servers for about an hour and it's all good. Before patching 2 or 3 minutes were enough to cause the problem.

Anyway having a regression test for that would be great.

[popravich]

Is there any possible way to test this proposed change?

For manual test I used docker scripts posted in related celery issue,
for automated testing — I'm not familiar with kombu that much(
Do you have any concurrency tests (multi-threaded or multi-processing)?

[popravich]

There are several issues in redis-py related to sockets shutdown in forked applications:
redis/redis-py#863
redis/redis-py#784

The reason why uninstalling hiredis package magically helps is another bug in redis-py:
PythonParser (which is used if hiredis is not importable) closes connection's socket before
Connection calls shutdown:
https://github.com/andymccurdy/redis-py/blob/master/redis/connection.py#L277-L285
I don't see any rational reason why parser should close non-owned socket, so, if PythonParser
will get fixed someday — uninstalling hiredis will stop fixing this bug :sarcams:

[wolph]

Excellent work @popravich! Thank you for chasing the bug down :)

I don't see any rational reason why parser should close non-owned socket, so, if PythonParser
will get fixed someday — uninstalling hiredis will stop fixing this bug :sarcams:

I fully agree. The big problem with this bug is/was how it manifested. It's not something you can handle from Python, it actually appears to be deadlocking in a bit of C code somewhere which is an issue. It completely stalls the Python interpreter which is something regular Python code should never be able to do.

[codecov]

Codecov Report

Merging #954 into master will decrease coverage by 0.07%.
The diff coverage is 25%.

@@            Coverage Diff             @@
##           master     #954      +/-   ##
==========================================
- Coverage   88.66%   88.58%   -0.08%     
==========================================
  Files          63       63              
  Lines        6512     6519       +7     
  Branches      777      778       +1     
==========================================
+ Hits         5774     5775       +1     
- Misses        656      661       +5     
- Partials       82       83       +1

Impacted Files	Coverage Δ
kombu/transport/redis.py	90% <25%> (-0.79%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fca408...efb7a44. Read the comment docs.

[liniribeiro]

Hello, This problem start to happen to me again after you revert 706f9f0

[auvipy]

try celery=>4.4.2